GoIP.de support #623
GoIP.de support #623
Conversation
There was a problem hiding this comment.
Hello, thanks for your contribution 💯
A few comments, especially:
- fix code formatting (you can use gofmt)
- change
domain->host, I made code suggestions to do that - more robust handling of the status ok case, so it's not tightly coupled with a specific response message that may change in the future.
Thanks!
My release notes are ready, but I'll wait for your PR to get merged first, so it's included in 2.6 😉
|
K. I'm done fighting the "Markdown/markdown" linter. But I think the code is sound. |
Added a new line at the end, to make the markdown checker happy.
.........and a trailing space
Please markdown linter, have mercy
mixed a "subdomain"
There was a problem hiding this comment.
git reset --hard origin/goip.de before doing anything, to be at the tip of the freshly rebased branch.
Now regarding the review 🔍, the two important notes/questions:
- In the end, we should (almost must) stick to
hostas suggested before. FQDN does not work with multiple FQDNs unlikehost(more repetition for the user) and does not fit with the rest of the code (aka will make my life harder refactoring in the future). So in the code this results in domain = "geoip.de" and host given as input. - Does goip.de support ipv6?
|
Finally, everything passes. |
- Remove seemingly copy-pasted errors from an irrelevant provider - No documentation found on status codes, so anything else than 200 is considered a fail - Change access denied error to wrap sentinel ErrAuth
|
I pushed a few changes/fixes, can you try the image |
|
I just tested with the credentials given in the original issue Username: toYRQs9GMuXdGJD And it works fine 😉 Merging this! |
|
Thx for adding. I deletet the Testlogin, works fine. |
Resolves #581