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

Add support for Dynu.com DNS/DDNS service #462

Merged
merged 28 commits into from
Jul 8, 2020
Merged

Add support for Dynu.com DNS/DDNS service #462

merged 28 commits into from
Jul 8, 2020

Conversation

chrisbraucker
Copy link
Contributor

Hello again,

I'm trying my best to support Dynu.com.

Though currently I'm stuck on seeding the integration test fixtures as

  1. I'm running into undocumented rate limiting (resulting in HTTP 503)
  2. The tests to add several records on either the example.com domain or with 127.0.0.1 fail serverside because the validation is (rightfully so) quite picky.

So the question is now: do I disable several tests, mock the data by hand (with example.com et al.) or change the test cases?

Also, #461 is not completely irrelevant, but won't affect this PR, just the handling of the CLI.

Best,
Chris

@CapsuleCD
Copy link
Contributor

Hi.

I'm an automated pull request bot named CapsuleCD. I handle testing, versioning and package releases for this project.

  • If you're the owner of this repo, you can click the button below to kick off the CapsuleCD build pipeline and create an automated release.'
  • If not, don't worry, someone will be by shortly to check on your pull request.

CapsuleCD

If you're interested in learning more about CapsuleCD, or adding it to your project, you can check it out here

@AnalogJ
Copy link
Owner

AnalogJ commented Nov 15, 2019

Hi @HerrFolgreich
Thanks for the PR! 🎉

When it comes to the integration test fixtures, what I usually do is use a real domain that I own to make the recordings (making sure to strip out the credentials from the header and body as sensitive data)

If you don't want your real domain name being published with the recordings, you can just do a find & replace after there recording is created, using example.com as the test domain.

Does that make sense?

@chrisbraucker
Copy link
Contributor Author

Hey @AnalogJ,

thanks for the help. Yep, that makes sense. I'll put in some code to avoid rate limiting and then see to getting those tests shipped :)

In that vein, do you think it would make sense to have a config file for the live test cases to set IP, domain and such?

@AnalogJ
Copy link
Owner

AnalogJ commented Feb 15, 2020

Hey @HerrFolgreich
I just wanted to follow up on this and see if you were able to get recordings working for your new provider?

@chrisbraucker
Copy link
Contributor Author

Hey @AnalogJ
thanks for asking. Still working on it. Was just a little busy the last weeks. I'll see to get it shipped this or next weekend :)

@chrisbraucker chrisbraucker changed the title [WIP] Add support for Dynu.com DNS/DDNS service, stuck on tests [WIP] Add support for Dynu.com DNS/DDNS service Feb 15, 2020
@chrisbraucker chrisbraucker force-pushed the master branch 4 times, most recently from 76de1cc to 763da33 Compare March 13, 2020 13:52
@chrisbraucker
Copy link
Contributor Author

This should be done now

@chrisbraucker chrisbraucker changed the title [WIP] Add support for Dynu.com DNS/DDNS service [Add support for Dynu.com DNS/DDNS service Mar 14, 2020
@chrisbraucker chrisbraucker changed the title [Add support for Dynu.com DNS/DDNS service Add support for Dynu.com DNS/DDNS service Mar 14, 2020
@chrisbraucker
Copy link
Contributor Author

Can we get this merged?

adferrand
adferrand approved these changes May 4, 2020
View reviewed changes
Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

Sorry I missed this PR! @AnalogJ was ok with it, and you added the integration tests. All good for me!

adferrand
adferrand requested changes May 4, 2020
View reviewed changes
Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

Oh sorry. It seems that the cassette for test_provider_when_calling_create_record_with_duplicate_records_should_be_noop is missing or invalid. Could you refresh it? I will merge right after.

@adferrand
Copy link
Collaborator

Hello @chrisbraucker, are you still willing to finish the PR?

@chrisbraucker
Copy link
Contributor Author

Hey @adferrand, I'll get this sorted out over the next couple of days :)

@chrisbraucker
Copy link
Contributor Author

@adferrand all green, finally :) thanks a lot for your help

@chrisbraucker
add create_record for dynu.com
0b85af9
@chrisbraucker
add delete_record for dynu.com
2fced41
@chrisbraucker
add _update_record for Dynu.com
cce1fd4
@chrisbraucker
fix delete record needs record_id
824d15a
@chrisbraucker
refactor record parsing
03eb1a4
@chrisbraucker
check lowercase fqdn for domain id
5f72f4c
@chrisbraucker
avoid connectioninterrupt by filtering possibly empty data
f986e49
@chrisbraucker
only add name field if not empty
4b4b9c6
@chrisbraucker
fix ref in record shape returned from dynu
aa69c93
@chrisbraucker
satisfy test that fails when domain not owned
f36f28c
@chrisbraucker
add rudimentary test hooks
1601d5e
@chrisbraucker
remove __class__ ref in favor of self
e83cf37
@chrisbraucker
Add dynu to supported providers
e0d69fb
@chrisbraucker
Add record mapping to/from dynu
5def372 
The Dynu API has very specific demands on how records
should look like. Where possible, mapping is done in
both directions to add support for records other than TXT
@chrisbraucker
Add myself to CODEOWNERS
17d5df5
@chrisbraucker
Update create to conform to spec
af78600
@chrisbraucker
Fix mapping to/from dynu api
e97314c
@chrisbraucker
Add test recordings
eaa3e89
@chrisbraucker
Fix failing test
62b212c
@chrisbraucker
Remove unused import
fb5bf2f
@chrisbraucker
Fix indentation
ac46260
@chrisbraucker
Unify logging
aa36088
@chrisbraucker
final pylint fixes
a4b323f
@chrisbraucker chrisbraucker requested a review from adferrand June 4, 2020 12:57
adferrand
adferrand approved these changes Jul 8, 2020
View reviewed changes
Copy link
Collaborator

@adferrand adferrand left a comment

Choose a reason for hiding this comment

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

LGTM!

@adferrand adferrand merged commit c909c94 into AnalogJ:master Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adferrand adferrand adferrand approved these changes

@AnalogJ AnalogJ Awaiting requested review from AnalogJ

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
@chrisbraucker @CapsuleCD @AnalogJ @adferrand