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 suport for Hetzner #503

Merged
merged 29 commits into from
Nov 17, 2023
Merged

feat(provider): add suport for Hetzner #503

merged 29 commits into from
Nov 17, 2023

Conversation

lieblinger
Copy link
Contributor

Based on cloudflare provider and related to #493

qdm12
qdm12 requested changes Jul 18, 2023
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.

Thanks 👍 👍

A few changes to be made 😉

docs/hetzner.md Outdated Show resolved Hide resolved
docs/hetzner.md Show resolved Hide resolved
internal/provider/headers/headers.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
lieblinger and others added 15 commits July 29, 2023 22:08
@lieblinger
[IMP] hetzner: Set Auth-API-Token in provider.go
61cd683
@lieblinger @qdm12
[IMP] hetzner: Remove uneccessary comment
5534ad4 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger @qdm12
[IMP] hetzner: Convert switch to regular if-statement
a655f35 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger @qdm12
[IMP] hetzner: Convert switch to regular if-statement
87eb753 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger @qdm12
[IMP] hetzner: Use netip.Addr instead of string on listRecordsResponse
f5e0b55 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger @qdm12
[IMP] hetzner: remove empty line
307685f 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger @qdm12
[IMP] hetzner: Remove comments
8f1d2c1 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger
[IMP] hetzner: Add check for empty response without id
87c3e22
@lieblinger @qdm12
[IMP] hetzner: Remove comments - 2
570ee44 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger
[IMP] hetzner: Set default ttl to 1
c1f3a57
@lieblinger
Merge branch 'master' of https://github.com/lieblinger/ddns-updater
da9642f
@lieblinger
[RM] hetzner: Remove key cuase it's not needed
edfcc92
@lieblinger @qdm12
[IMP] hetzner: Refactor getRecord function
6276a93 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger
[IMP] hetzner: Unexport Create function
7a78a25
@lieblinger
Merge branch 'master' of https://github.com/lieblinger/ddns-updater
eb8b21c
@lieblinger lieblinger requested a review from qdm12 August 1, 2023 07:54
qdm12
qdm12 reviewed Aug 4, 2023
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.

Thanks for the changes 🎖️! A few minor comments and it should be good to merge 😉

internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
@qdm12
Copy link
Owner

qdm12 commented Aug 4, 2023

Also:

  • linting is failing, try running golangci-lint run locally to check why? (or check the verify job log output)
  • maybe, you could move each long function to its own file (createrecord.go, getrecord.go, updaterecord.go). Not 100% compulsory, but I used such split in other providers and it's nicer to navigate in my opinion.

@lieblinger
Copy link
Contributor Author

lieblinger commented Aug 14, 2023 •
edited
Loading

Thanks for your review!

I've made some changes and hope that it's okay now. If there's anything else that needs to be changed just let me know.

@lieblinger
Copy link
Contributor Author

@qdm12 Do we need to change anything else?

@varac
Copy link

varac commented Sep 28, 2023

I tested this PR using an image built from this PR banch (registry.0xacab.org/varac-projects/images/ddns-updater:hetzner) and it works well 🍿
Looking forward to get this merged soon.

qdm12
qdm12 reviewed Oct 10, 2023
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.

Sorry for the massive delay reviewing 😢
A final review, it shouldn't be too complex to fix, let me know your thoughts.
Feel free to only comment back on new conversations, I can finish up the coding fixes if you don't have time, especially since it has been a few weeks since this was opened sorry!

internal/provider/providers/hetzner/common.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/create.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/create.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/getrecord.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/getrecord.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/create.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/provider.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/update.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/create.go Outdated Show resolved Hide resolved
internal/provider/providers/hetzner/update.go Outdated Show resolved Hide resolved
@lieblinger @qdm12
[IMP] Hetzner: Path variable
168e864 
Co-authored-by: Quentin McGaw <[email protected]>
@lieblinger lieblinger requested a review from qdm12 October 10, 2023 15:03
@lieblinger
Copy link
Contributor Author

Thanks for your review. All changes are made and I hope that everything is okay.
If there's something else I can update just let me know.

qdm12
qdm12 approved these changes Oct 11, 2023
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.

Awesome 💯
Just two last tiny double checking questions 😉 And then let's merge finally 👍

internal/provider/providers/hetzner/getrecord.go Show resolved Hide resolved
internal/provider/providers/hetzner/create.go Show resolved Hide resolved
@lieblinger lieblinger requested a review from qdm12 October 25, 2023 16:55
@qdm12 qdm12 merged commit 301e4d6 into qdm12:master Nov 17, 2023
5 checks passed
@varac
Copy link

varac commented Nov 24, 2023

@qdm12 Do you mind doing a new release with this feature ? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nabilelmahiri nabilelmahiri nabilelmahiri approved these changes

@qdm12 qdm12 Awaiting requested review from qdm12

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

Successfully merging this pull request may close these issues.
4 participants
@lieblinger @qdm12 @varac @nabilelmahiri