Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(provider): add support for Vultr #829

Merged
merged 40 commits into from
Oct 20, 2024
Merged

feat(provider): add support for Vultr #829

merged 40 commits into from
Oct 20, 2024

Conversation

amroessam
Copy link
Contributor

@amroessam amroessam commented Oct 2, 2024 •
edited
Loading

First time writing go, this was easy thanks to great docs and example 🙏🏽
Go easy on the roast 😛

Vultr DNS API docs for ref

Excited for the merge, so I can use this instead of my janky script https://github.com/amroessam/vultr-ddns

@amroessam
Copy link
Contributor Author

Preemptively answering these questions

  1. Yes, the default behavior is to create the record if it doesn't exist
  2. Yes, I've tested it
  3. Wildcards are supported but only for 1 record type, if it errors out it should reflect the error

@amroessam
Copy link
Contributor Author

@qdm12 any luck getting to this? 🙏🏽

qdm12
qdm12 requested changes Oct 8, 2024
View reviewed changes
Copy link
Owner

@qdm12 qdm12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! Thanks for the PR and congrats on your first few Go steps! 💯
Looking pretty good, with a few minor changes to make 😉

.gitignore Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
internal/provider/providers/vultr/provider.go Show resolved Hide resolved
internal/provider/providers/vultr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/vultr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/vultr/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/vultr/provider.go Outdated Show resolved Hide resolved
qdm12
qdm12 reviewed Oct 9, 2024
View reviewed changes
internal/provider/providers/vultr/provider.go Outdated
decoder := json.NewDecoder(response.Body)
var parsedJSON struct {
Error string
Status uint32
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also what is that Status used for? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the HTTP status code

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's weird 😄 Should we... remove it from that struct?? Since it's... well, in the HTTP response already...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of implementing the API, I've added it, can remove it if you insist 🙏🏽

qdm12
qdm12 reviewed Oct 9, 2024
View reviewed changes
internal/provider/providers/vultr/provider.go Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
docs/vultr.md Outdated Show resolved Hide resolved
@amroessam
Copy link
Contributor Author

Sorry for the delay, I was away. Can you recheck?

@qdm12 qdm12 force-pushed the master branch 2 times, most recently from 126315b to 691ed32 Compare October 20, 2024 13:15
qdm12 and others added 21 commits October 20, 2024 16:21
@qdm12
chore(deps): tidy go modules
dff1ba7
@amroessam @qdm12
Add dev files to gitignore
694319a
@amroessam @qdm12
Add vultr provider
c7117ee
@amroessam @qdm12
Add vultr docs
faac595
@amroessam @qdm12
Add vultr to provider choice
ad3116a
@amroessam @qdm12
Remove debug message
bf3680c
@amroessam @qdm12
Update docs/vultr.md
e01f8bc 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update docs/vultr.md
425200b 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update internal/provider/providers/vultr/provider.go
b31be3c 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update internal/provider/providers/vultr/provider.go
e7f0710 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update internal/provider/providers/vultr/provider.go
80942df 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update error for token not set
2694d6f
@amroessam @qdm12
Remove checking for ttl
0cc3a71
@amroessam @qdm12
Revert gitignore
c43db65
@amroessam @qdm12
Added info about wildcard
947193f
@amroessam @qdm12
Switch to apiKey instead of Token
a68afad
@amroessam @qdm12
Update internal/provider/providers/vultr/provider.go
ce6ef3f 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update docs/vultr.md
f2dd4bc 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Update docs/vultr.md
77ada26 
Co-authored-by: Quentin McGaw <quentin.mcgaw@gmail.com>
@amroessam @qdm12
Add update record and define record type
af8aedf
@amroessam @qdm12
Add switch case for create record response
8d6d608
amroessam and others added 17 commits October 20, 2024 16:26
@amroessam @qdm12
Add pagination comment
0364369
@qdm12
vultr.md: small rewording here and there
85eb99b
@qdm12
formatting: use gofumpt
4339602
@qdm12
chore: add API documentation url for each function
e2626b7
@qdm12
chore: use Data struct field name for "data" json fields
c6222bd
@qdm12
fix: getrecord to match a single record type
52e59a7
@qdm12
fix: return an error if record is not found
1c34925
@qdm12
chore: updateRecord does not return a newIP
74c2127
@qdm12
chore: get rid of record struct in favor of narrower fields
9219ef0
@qdm12
chore: remove the use of undocumented field Status
9d46d56
@qdm12
fix: associate error fields with error json key
eb5001f
@qdm12
feat: parseJSONError returns the whole body if decoding fails or pars…
f263792 
…ed error is empty
@qdm12
fix: check error field even on successful status codes
33fd1f7
@qdm12
chore: fix last lint error
4c3e064
@qdm12
chore: createRecord does not check response ip against sent ip anymore
e46309e 
- we don't do it for updateRecord
- not really sure why we do it in the first place honestly
@qdm12
chore: minor changes and formatting
459e397
@qdm12
fix: getRecord handles response bodies after decoding it once
a7e9b0a
@qdm12
Copy link
Owner

qdm12 commented Oct 20, 2024

Hey there, thanks for the last few commits!
In the end I decided to push directly on your branch, since there were quite a few things to change (ranging from not important at all to somewhat important - see commits with fix: prefix). Feel free to browse commit by commit, since I tried my best to separate changes one by one for knowledge sharing purposes!
I also rebased your branch on the master branch here, so feel free to get changes using git checkout master; git fetch; git reset --hard origin/master 😉
Just waiting for the CI to pass and I'll merge this. Thanks!

@qdm12 qdm12 merged commit 949dcd9 into qdm12:master Oct 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qdm12 qdm12 qdm12 requested changes

@ehab-qadah-lean ehab-qadah-lean ehab-qadah-lean approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amroessam @qdm12 @ehab-qadah-lean