-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fixes around go get and cleaning up #23
Conversation
This fixes grafana/xk6-output-kafka#7 (comment) |
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.
The cleanup change LGTM, but I'm not clear on why the go get
change is needed. Can you explain it here and in the commit itself?
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.
LGTM, though yeah, some explanations in the PR description will be great. Also, I can't find it in the docs, but does go mod edit -require
need Go 1.16 or would it also work on 1.15?
I don't really know why
I think it works ... for even lower versions but that is irrelevant as we use |
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.
Strange... Yeah, seems like a bug related to the dependency changes we did when moving out the Kafka output, but I couldn't find anything on the issue tracker about it.
I confirmed the fix locally, so hopefully it won't break anything else 😅🤞
From some chatting in the |
go get
seems to actually try to set and get the exact version you provide and so if it can't that is an error.On the other hand
go mod edit -require
mostly adds it to the requirements and then the module resolution algorithm just removes it as there is actually a higher version already required effectively doing what we want.