-
Notifications
You must be signed in to change notification settings - Fork 153
Add VadlidatingAdmissionPolicy RBAC and bump k8s to v1.30 release #3564
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
This is changed in upstream PR #6479. @rene-dekker I think this might be the most clean way to adapt to the vendor change. WDYT?
Uh oh!
There was an error while loading. Please reload this page.
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.
We effectively only use this variable in 1 place. I think it would be better to convert the field where it is used, so that we don't need to make breaking changes to our API.
Maybe in pkg/ptr/conversion.go we can add a utility function converting pointer slices to slices using generics?
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.
Jiawei reminded me that this is not really a breaking change. Everything that is not an array containing nil values should deserialize properly. That paired with the knowledge that this feature is quite new and not used so much yet, makes me think that it is probably ok.
WDYT: @tmjd @caseydavenport
Uh oh!
There was an error while loading. Please reload this page.
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 it is OK, but I do also think it is a breaking API change.
For one, anyone importing our code using this field (doubt that's anyone!) will fail to compile when they bump the version.
Technically the nil value array is still a breaking change, although we can probably make a case that nobody is using that config since it wouldn't have worked before anyway (but our API would have accepted it)
All this to say that I'm OK with making this change to make our lives easier, even though technically it's a breaking change.
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.
Seems ok to me