-
-
Notifications
You must be signed in to change notification settings - Fork 164
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
Added Now-DNS issue#368 #504
Conversation
Crap. Line 137 I left a "debugging statement" in there 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, thanks 💯 !
Just a few minor changes to be made, thanks 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add nowdns to the list in the readme.md, and add a document docs/nowdns.md
Apparently in my sleep deprived state I neglected to add this to my last commit 😬 |
Changes made. Docker build still failing though.
|
The build fails because of |
Linux mint 5.15.0-76-generic #83-Ubuntu SMP Thu Jun 15 19:16:32 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux How would I override it? edit: Well, if I delete the GOARCH and GOARM lines, it builds just fine. 😂 |
Fixed the Dockerfile. The Edit: But referencing the Dockerfile from other repos, apparently it is supposed to build just fine with TARGETPLATFORM being blank. So IDK. |
Okay, builds inside of a GitHub codespace container just fine with the original Dockerfile. Build output below.
|
That's most likely because your local docker is a bit outdated or doesn't use buildkit/buildx, which should set TARGETPLATFORM to Now I'm trying to figure out how to prevent this issue arising again on such Docker setups. First, there is a bug where TARGETPLATFORM cannot have a default in the Dockerfile: docker/buildx#510 so setting it to As it is, xcputranslate fails and exits with 1, for example
But because it's wrapped in a subshell in There is also an alternative where xcputranslate handles the empty TARGETPLATFORM string and outputs the current platform, but that's a bit of a pain to implement and reliably detect the platform version, so I would rather avoid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did that and it was failing to update the record.
It has to be passed as "@", otherwise the API throws a "nohost" error.
Fair point. Fixed.
If the host can only be @
(not *
or abc
), then remove the option host
from the config and the field host
in the provider struct.
But without host field, it fails. "nohost" is what the API returns.
edit: I see the issue. Without the `host` field set to `@`, a period gets added to the front of the fqdn somewhere, causing the error.
|
This opens up a whole lot of other issues, like having to refactor host out of the rest of the file, redefine or replace utils.ToString(p.domain, p.host, constants.NowDNS, p.ipVersion) as a variadatic function. I think I found a simpler solution that will be in the next commit that I think will work just as well. |
…dance for end user
Basically just set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the solution is good, simple and ready to merge into master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, great work 💯 Merging it 😉
side note: I went ahead and removed the host injection and host field in the struct (hardcoding @
where needed), since that was itching my brain too much to have a constant struct field and unused argument injected.
Fixes #368