Skip to content

Fix DNSExit provider when provided with a zone and a non-identical hostname #674

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
Merged
merged 4 commits into from
Jun 2, 2024
Merged

Fix DNSExit provider when provided with a zone and a non-identical hostname #674

merged 4 commits into from
Jun 2, 2024

Conversation

jortkoopmans
Copy link
Contributor

@jortkoopmans jortkoopmans commented May 19, 2024 •
edited by rhansen
Loading

This PR proposes a fix for issue #673. The fix involves trimming the zone from the hostname in the request. In addition to the bug fix, this PR includes refactoring for readability and the addition of a testing harness.

  • Trimmed the zone from the hostname in the request to fix issue.
  • Refactored code for improved readability.
  • Added a testing harness which includes:
    • Moving JSON support loading into the sub for testing.
    • Creating a config loader helper sub.

Testing

The actual bugfix is fairly limited, however, I ended up refactoring quite a lot to be able to write some tests. The current architecture of the application made it challenging to test the provider implementation due to:

  • The %config hash being modified globally within the ddclient script (by several subs).
  • The functions (subs) in ddclient not being exported as part of a module, making it difficult to use these functions in tests (outside of the script).

Despite these challenges, I've included some tests in this PR. I understand that this may trigger a discussion about how to improve the structure of the software, which I hope will be to the benefit of this program altogether.

fixes #673

@jortkoopmans jortkoopmans force-pushed the bugfix/673_Fix_DnsExit_subdomain branch from 633df13 to 5b63b88 Compare May 19, 2024 16:01
rhansen
rhansen requested changes May 20, 2024
View reviewed changes
Copy link
Member

@rhansen rhansen 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 contribution!

@jortkoopmans jortkoopmans force-pushed the bugfix/673_Fix_DnsExit_subdomain branch 6 times, most recently from b08acdb to ab56cde Compare May 27, 2024 00:48
@jortkoopmans jortkoopmans requested a review from rhansen May 27, 2024 00:58
This was referenced May 28, 2024
Closed
Merged
@rhansen rhansen force-pushed the bugfix/673_Fix_DnsExit_subdomain branch 6 times, most recently from aae2dab to 4fb98cd Compare May 31, 2024 07:28
@jortkoopmans
Copy link
Contributor Author

Amazing work @rhansen 🤩 , it's looking great!
Good solution to revert to empty name by smart trimming, that simplifies the approach. And generally more concise coding 😃 .
I have added a minor improvement, but more importantly I've tested the solution on the DNSExit API and it works fine.

@rhansen rhansen force-pushed the bugfix/673_Fix_DnsExit_subdomain branch from c16af73 to 8dd811f Compare May 31, 2024 23:08
rhansen
rhansen approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@rhansen rhansen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jortkoopmans Thanks for reviewing. I squashed all of the commits together then split into four:

  1. Add tests (marked as expected to fail)
  2. Fix bug (and mark tests as expected to pass)
  3. Don't skip remaining hosts on HTTP error. This commit exists mostly to simplify the next commit: a next can be simply converted into a return whereas last needs additional logic in the calling function.
  4. Move the body of the for loop to a separate function

I split this PR into multiple commits for the following reasons:

  • It makes it easier for downstreams to cherry-pick a minimal diff to fix the issue.
  • It makes it easier for future developers to see what changed.
  • git bisect and git revert will be more useful if this PR breaks something.

Please review the commits one last time (especially the commit messages), make any changes you deem appropriate (force push is OK), and let me know if everything looks OK.

@jortkoopmans
Copy link
Contributor Author

LGTM @rhansen . I have reviewed the commits, commit messages and re-tested the final solution just to be sure; all is good.

jortkoopmans and others added 4 commits June 2, 2024 16:58
@jortkoopmans @rhansen
dnsexit2: Add tests
ec2d5f7 
Needs LWP::UserAgent.
@jortkoopmans @rhansen
dnsexit2: Fix when provided with a zone and a non-identical hostname
216741c 
Trim the zone from the hostname in the request to fix issue.
@rhansen
dnsexit2: Don't skip remaining hosts on connect error or non-2xx
11d0c84 
A non-2xx status code might be host-specific, so ddclient should
continue with the next host.  We could skip the remaining hosts if
there is a connection failure, but it doesn't hurt to retry.
@jortkoopmans @rhansen
dnsexit2: Move body of for loop to a separate function
73a67b7 
For improved readability.
@rhansen rhansen force-pushed the bugfix/673_Fix_DnsExit_subdomain branch from 8dd811f to 73a67b7 Compare June 2, 2024 20:58
@rhansen rhansen merged commit 09ce262 into ddclient:master Jun 2, 2024
@jortkoopmans jortkoopmans mentioned this pull request Jan 9, 2025
Open
@chenrui333 chenrui333 mentioned this pull request Jan 20, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhansen rhansen rhansen 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.

DNSExit: Updater breaks when provided with a zone and a non-identical hostname (bug)
2 participants
@jortkoopmans @rhansen