-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
upgrade kustomize-in-kubectl to v5.4.2 #124217
base: master
Are you sure you want to change the base?
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Apr 7 07:41:51 UTC 2024. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
I'm pretty sure the CreditCard / URLIP / URL regex changes in this file change format validators used in kube-openapi and therefore in CRD validation in ways that can break cross-version validation of persisted data
I don't think we should bump this until CR ratcheting is present in all supported versions.
Options are to figure out what pulled in this new version and revert it, or snapshot the bits of govalidator we use into kube-openapi to lock our format validation API surface. I'm more in favor of the latter.
cc @sttts @alexzielenski for visibility
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.
I think we can revert the version of the github.com/asaskevich/govalidator
pkg.
kubernetes-sigs/kustomize#5680
I restored that in master branch, so Could you wait to create a new release for kustomize?
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.
Hi @liggitt
We restored the github.com/asaskevich/govalidator
version.
Could you recheck this PR?
/remove-sig api-machinery |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/remove-sig instrumentation |
….17.2, kustomize/v5.4.2
/retest |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
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.
thanks, the govalidator revert lgtm
just a couple questions on pulling in untagged versions of a couple deps, then lgtm
@@ -50,12 +50,12 @@ require ( | |||
github.com/opencontainers/runc v1.1.12 | |||
github.com/opencontainers/selinux v1.11.0 | |||
github.com/pkg/errors v0.9.1 | |||
github.com/pmezard/go-difflib v1.0.0 | |||
github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 |
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 it intentional this is pulling in a version between tags? do we know if this library expects to be used on a non-tagged version and is stable there?
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, I didn't notice the two non-tag dependencies added.
I investigated those dependencies and found those two dependencies used in github.com/spf13/viper@v1.17.0
.(kubernetes-sigs/kustomize#5541)
https://github.com/spf13/viper/blob/v1.17.0/go.mod#L32
https://github.com/spf13/viper/blob/v1.17.0/go.mod#L61
I think kustomize is still working from those packages that are upgraded due to all tests being passed.
But if you have concerns to make effect other things from the update of those dependencies, I think I can consider downgrading the github.com/spf13/viper/
package, too.
@@ -139,7 +139,7 @@ require ( | |||
github.com/containerd/ttrpc v1.2.2 // indirect | |||
github.com/coredns/caddy v1.1.1 // indirect | |||
github.com/coreos/go-semver v0.3.1 // indirect | |||
github.com/davecgh/go-spew v1.1.1 // indirect | |||
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // 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.
is it intentional this is pulling in a version between tags? do we know if this library expects to be used on a non-tagged version and is stable there?
I realized we have 3 parallel PRs attempting to update kustomize... (#124812 #124217 #123339) #123339 is the oldest and also took care of switching k/k to the v4 json-patch library to avoid using two names for the same library I really appreciate the work here to get the openapi validator change switched back in kustomize, but I think we should proceed with merging #123339 now) |
/remove-sig instrumentation |
/remove-sig api-machinery |
What type of PR is this?
What this PR does / why we need it:
This upgrades kustomize-in-kubectl to v5.4.2
Which issue(s) this PR fixes:
Fixes kubernetes/kubectl#1572
Special notes for your reviewer:
This is a go.mod change to upgrade the kustomize
dependency from v5.0.4 to v5.4.2
Does this PR introduce a user-facing change?