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.

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

Add Hover as DNS-style provider #1255

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chickenandpork
Copy link

Support for Hover (http://hover.com/) (was: TuCows) as a DNS provider

This method mimics a HTTP client because Hover lacks a formal/public API. The work is based on adapting some reverse-engineering into Go.

@ldez

This comment has been minimized.

@ldez
Copy link
Member

ldez commented Sep 23, 2020

Hello,

If this provider doesn't have an API and/or official API documentation, I'm not sure we will agree to add this provider.

Also https://github.com/chickenandpork/hoverdnsapi has a dependency on lego, we will not allow that.

@ldez
Copy link
Member

ldez commented Sep 23, 2020

Here is what I recommend:

  • create a document describing the API and put it in a repository or in lego
  • create in lego a light client instead of an external library

Without this, it will not be possible for us to accept your PR.

@chickenandpork
Copy link
Author

I'm not sure we will agree to add this provider

Does this mean literally "I am not sure", or is it a soft way of saying "we will not" ? If the PR has zero chance of ever being merged, then we should just abandon it now, but if there's a chance I'll be able to use it to maintain the DNS records I use on my services, that would be immensely useful, hence I'd prefer to continue if there's a chance of being accepted.

@ldez
Copy link
Member

ldez commented Sep 23, 2020

When I said "I am not sure", I meant that I needed time to think about it.

For the rest, I think I answered in my previous comment #1255 (comment)

If you want to use your external library you will have to modify it in order to remove lego dependencies. But I would really prefer to have a minimalistic implementation of the client in lego.

@ldez ldez self-requested a review September 23, 2020 23:50
@ldez
Copy link
Member

ldez commented Sep 25, 2020

Do my answers allow you to make progress on the subject?

@chickenandpork
Copy link
Author

chickenandpork commented Sep 25, 2020

Do my answers allow you to make progress on the subject?

TL;DR: yes, sorry to go silent. Will attempt this weekend.

I appreciate that you're blunt and clear; none of this USA "I don't think" meaning actually a polite "heck no!". I understand if my implementation can lead to later maintenance concerns.

Honestly, my immediate response was somewhere between "aw shoot, I need to discard the work and start again" with the comment that was made at the same time mine was, and was unsure what example of a "light client" I could follow to make it as consistent as possible. I mean: the words make sense, but it seems there's a specific example in mind, and consistency helps review.

...but I see that you're willing to go forward -- maybe at least to get the functionality there? -- with the external library if I can clean it up. If I remember correctly, there was some import of types to ensure compatibility, but I can likely remove those.

I can try to move forward this weekend if that's OK, and I do really appreciate the feedback, I regret going silent for a while.

@chickenandpork chickenandpork force-pushed the 20200501-dns-type-provider-for-tucows-hover branch from 69d3e07 to 86ffa79 Compare September 1, 2021 07:34
@chickenandpork
Copy link
Author

Hi; Took me a while to move the dependent code to a ./internal/ to break the dependency -- changed country, changed jobs, hardware packed up shipping containers forever, etc.

This update is rebased, has the external dependency added, and unit tests OK. The underlying code still functions fine as a CLI tool as well before copying tinto this module.

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

Successfully merging this pull request may close these issues.

None yet

2 participants