Skip to content
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

🌱 Bump golang.org/x/text to v0.3.7 #2438

Merged
merged 1 commit into from
Dec 5, 2021
Merged

🌱 Bump golang.org/x/text to v0.3.7 #2438

merged 1 commit into from
Dec 5, 2021

Conversation

pjbgf
Copy link
Member

@pjbgf pjbgf commented Dec 2, 2021

Description

Bump an indirect dependency to fix minor security advisory which was fixed a few months ago.
More information in the linked issue below.

Fixes #2437

Signed-off-by: Paulo Gomes <pjbgf@linux.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 2, 2021
@pjbgf
Copy link
Member Author

pjbgf commented Dec 2, 2021

@camilamacedo86 following up the meeting today, would you mind taking a look at this PR please?

Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK this won't actually change the version used by dependencies, and kubebuilder doesn't use this dep directly. Use a replace directive instead

replace golang.org/x/text => golang.org/x/text v0.3.7

@pjbgf
Copy link
Member Author

pjbgf commented Dec 3, 2021

@estroz Thank you for reviewing this PR. I believe the difference between the two approaches is that one defines the minimum version of the indirect dependency, and the other replaces all references to it.

For example, the current approach relies on golang MVS to find the minimum required version on go.sum, which in this case would be v0.3.7:

golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.1-0.20180807135948-17ff2d5776d2/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk=
...
golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=

If we were to use replace instead (e.g. replace golang.org/x/text => golang.org/x/text v0.3.7), the go.sum would look like:

golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk=
golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ=

Meaning that in the future when our direct dependencies start requiring a version newer than that, it would still be replaced by v0.3.7. This approach would act as a pin so to speak.

Please let me know whether that's still the preferred approach and I will amend the PR accordingly.

PS: This was already tested on security-profiles-operator with the desired effect confirmed through deps.dev.

@camilamacedo86
Copy link
Member

camilamacedo86 commented Dec 3, 2021

HI @rashmigottipati and @estroz,

Following my review:

First, the mojo here would be :seedling: that is not a new feature.
I do not think that is the case to use the replace.

However, why do we need to bump only this indirect dep?

Could we not just:

  • Remove all indirect ones
  • Bump go 1.17
  • Run go mod tidy

And then, update all indeed the go version which is a requirement tracked in the repo? Would not that make more sense?

/hold

Could we solve the need to update this dep by updating the go version? Is that OK for you @rashmigottipati? WDYT?

@pjbgf pjbgf changed the title ✨ Bump golang.org/x/text to v0.3.7 🌱 Bump golang.org/x/text to v0.3.7 Dec 3, 2021
@estroz
Copy link
Contributor

estroz commented Dec 3, 2021

@pjbgf I was not aware that // indirect declarations in require made a difference, but you are right

$ go list -m all | grep golang.org/x/text
golang.org/x/text v0.3.7

I'm ok with this change. @camilamacedo86 in go 1.17 the indirect's are listed in a separate block so this won't be an eyesore after bumping go versions, so we can merge this then bump in a separate PR. Thoughts?

Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approved
/lgtm

For me @rashmigottipati @estroz

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, pjbgf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 0036a35 into kubernetes-sigs:master Dec 5, 2021
@pjbgf pjbgf deleted the bump-text branch December 9, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bump golang.org/x/text to v0.3.7
4 participants