-
Notifications
You must be signed in to change notification settings - Fork 51
Conversation
6b4f4cf
to
f43ed1f
Compare
Thanks for your contribution! (I cleaned up my comments to reduce confusion. See the code review below) |
I am on vacation with limited access, but from what I see I fully agree with you @bmaupin especially on the maintenance of the CI dependencies (with the commit numbers). |
I've been out of the Go world so long, I didn't realize that updating dependencies would force the version of Go in this project to be updated as well. So that explains many of the changes here. I guess that technically still counts as a breaking change; if it means the library would require Go 1.20, then I guess we'd need to bump the major version of this library to v2. Which shouldn't be too big of a deal, it's just a version number :) I'll try to address any remaining issues by adding code reviews. |
As general comment, I think is best if an http server is used during the tests for the URL downloads rather than downloading files from the internet, but I don't think you want me to address a change like that here? |
I agree 100%. Yes, for now I think we can limit the changes to fixing the tests and upgrading dependencies. Once that's done, I think it would be a great idea to use an HTTP server during the tests. That would prevent broken URLs from breaking the tests in the future. Feel free to submit a PR for it if you'd like. |
This reverts commit 772d6fa.
I have fully reverted the changes to tests (because now the URLs works for me? Don't know what happened, but tests where failing for me on requests 🤷). I can do separate PRs to move the tests to an http server and to fix all deprecations being used, maybe even add a golangci-lint to actions as well. |
Thanks so much, that sounds great! Thanks for your contribution! |
Go updates
Test fixes
testify/require
in some non-checked scenariosioutil
only in the tests I modifiedCI updates
go.mod
file.