-
Notifications
You must be signed in to change notification settings - Fork 32
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
chore(deps): Upgrade golang.org/x/net and golang.org/grpc #30
Conversation
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.
Sorry for the delayed review and thanks for separating your PRs
go.mod
Outdated
golang.org/x/sys v0.3.0 // indirect | ||
golang.org/x/text v0.5.0 // indirect | ||
google.golang.org/genproto v0.0.0-20221207170731-23e4bf6bdc37 // indirect | ||
golang.org/x/net v0.17.0 // indirect |
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.
instead of updating indirect
dependencies, could you add those in a replace
section with a comment explaining the reason for it.
// Update Go Networking to avoid CVE-2023-44487 and CVE-2023-39325
replace golang.org/x/net => golang.org/x/net v0.17.0
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.
Is there a specific reason to use replace instead go get?
imho go get
is better since it will also update go.sum entries and update related dependencies as well.
And, i guess, replace is more used for development purposes.
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.
Well, I suppose that there is some developer preference involved here, but in general, the idea is consistency and reproducibility from environment to environment (development, production, CI/CD, Docker builds). We really don't want to use go get ...
unless we deliberately want to add new dependencies, or update individual dependencies, which will cause the go.mod
and go.sum
file to be updated.
When I set up a Go project, meaning, I fork a GitHub repo and clone it locally, I will then run go mod download
to get the dependencies with the exact versions as specified, or go mod tidy -compat=1.18
(for a Go 1.18 project). The latter should not change the go.mod
or go.sum
file either, if the the two files have been kept in order.
I expect only the direct dependencies to be maintained. The list of // indirect
dependencies should be "generated" by running go mod tidy
(or go mod tidy -compat=1.18
-- for a Go 1.18 project) which will determine the versions of those indirect dependencies based on the versions required by the modules from the list of "direct" requirements.
Now, how should we "override" certain indirect dependencies in the go.mod
file? I think we should not just change a version of a indirect dependency, i.e. by running go get ...
which will change the list // indirect
dependencies. This would make it difficult to determine in the future, which versions we decided to use, versus what versions were required indirectly.
To keep that distinction clear, Go offers the replace
directive.
Ah, and go mod tidy ...
will also update go.sum
entries and update related dependencies ;-)
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.
Yeah, make sense, thanks.
20bb788
to
4f7dcc9
Compare
btw, just tested, the |
chore(deps): Upgrade golang.org/x/net and golang.org/grpc Signed-off-by: Spolti <[email protected]>
Signed-off-by: Spolti <[email protected]>
4f7dcc9
to
eb14be6
Compare
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.
/approve
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.
Great. Thanks @spolti
/lgtm
/approve
…b-io#30) Issues addressed: - https://github.com/kserve/rest-proxy/security/dependabot/1 - https://github.com/kserve/rest-proxy/security/dependabot/2 - https://github.com/kserve/rest-proxy/security/dependabot/3 - https://github.com/kserve/rest-proxy/security/dependabot/4 - https://github.com/kserve/rest-proxy/security/dependabot/5 - https://www.cve.org/CVERecord?id=CVE-2023-37788 --------- Signed-off-by: Spolti <[email protected]>
…b-io#30) Issues addressed: - https://github.com/kserve/rest-proxy/security/dependabot/1 - https://github.com/kserve/rest-proxy/security/dependabot/2 - https://github.com/kserve/rest-proxy/security/dependabot/3 - https://github.com/kserve/rest-proxy/security/dependabot/4 - https://github.com/kserve/rest-proxy/security/dependabot/5 - https://www.cve.org/CVERecord?id=CVE-2023-37788 --------- Signed-off-by: Spolti <[email protected]>
…b-io#30) Issues addressed: - https://github.com/kserve/rest-proxy/security/dependabot/1 - https://github.com/kserve/rest-proxy/security/dependabot/2 - https://github.com/kserve/rest-proxy/security/dependabot/3 - https://github.com/kserve/rest-proxy/security/dependabot/4 - https://github.com/kserve/rest-proxy/security/dependabot/5 - https://www.cve.org/CVERecord?id=CVE-2023-37788 --------- Signed-off-by: Spolti <[email protected]>
Issues addressed: