-
Notifications
You must be signed in to change notification settings - Fork 1.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
(feat) add new validator for deprecated apis #5216
(feat) add new validator for deprecated apis #5216
Conversation
1c4f040
to
92a957f
Compare
Signed-off-by: Camila Macedo <[email protected]>
92a957f
to
0aaa2de
Compare
k8s.io/cli-runtime v0.21.0 | ||
k8s.io/client-go v0.21.2 | ||
k8s.io/client-go v0.22.1 |
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 haven't bumped the dependencies to 1.22 yet, since kb didn't bump it yet. Though this just bumps the deps in SDK binary and not in the plugin, do we still wait till that is done?
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 certainly different than what we've done in the past right? @camilamacedo86 is this really needed for this 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.
The API is bumped. So, when we bump the API that is automatic.
KB does not use these deps in the CLI tool as well.
It has only k8s.io/apimachinery v0.21.3
which is used for the alpha-gen.
If I am not wrong, we needed first to ensure the controller runtime was updated.
Then, we could bump the scaffolds in KB and then here.
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 can fix it too :-P :
- To upgrade only the CLI tool used as here:
🌱 upgrade CLI tool dep k8s.io/apimachinery from v0.21.2 to v0.22.1 used by alpha config-gen kubernetes-sigs/kubebuilder#2339
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.
As long as we're:
- not somehow using the deps of operator-sdk itself in the code that SDK scaffolds
- able to pin our test environments (kind, envtest, etc.) to use the versions we want, not the versions of SDK's deps
I don't see how this API bump would be a problem. Since no one should be able to import SDK code anymore, this should have no affect on anything except the operator-sdk binary itself.
Is there a specific reason we know of that forces us to keep aligned with kubebuilder and/or our scaffolded code?
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.
Thank you for your input and help 🥇
I think the concern only comes because usually, we bumped KB first to also get the scaffolds updated as well.
@jmrodri @varshaprasad96 could we move forward?
e62cf8c
to
0aaa2de
Compare
This PR itself looks good to me. I'm more curious about the validator logic in the API repo. What's with the Seems like that should be either INFO level or not logged at all (the caller explicitly asked for it, so IMO it just adds noise to the output) Also, how does this validator work. Can I pass any kubernetes version I want? Can you link the PR where the validator was merged? |
Hi @joelanford, Thank you for your time here and help. Following the answers to your comment inline:
This implementation has been used for a while in many places such as audit,iib, CVP. Also, it was QE tested as well and provided by SDK since its 1.18.0 version. It has been proved very helpful to check the usage of the removal APIs in 1.22 since the most likely Operators projects to be affected are shipped on the bundle (CRD/RBAC). Users have been able to take advantage of these checks via the optional operatorhub validator already. This PR/new validator only allows them to test their bundles exclusively against this criteria when it is not desirable to check the whole criteria provided by operatorhub optional validator.
That is not necessary at all. The user informed the version already. I did PR to address this nit: operator-framework/api#157
Currently, it only has implemented checks for the k8s version 1.22, but you can pass any version. We will add checks for 1.25 as well (probably, least effective in this case). Would be nice we tell what are the supported versions. That would be easier after we are able to address something like operator-framework/api#158. I'd not like to add to the consumer its seems to make more sense to be in the validator itself. The code is here: https://github.com/operator-framework/api/blob/master/pkg/validation/internal/removed_apis.go, and it has a hall for improvements as well, which should get done soon when we will add the checks for 1.25 or as its follow up. Brett and/or I will be working to improve its maintainability. |
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
Description of the change:
Allow users checks their bundle against the specific criteria over deprecation of APIs in k8s
Motivation for the change:
The deprecation of APIs in k8s 1.22 impacts the operator's projects
When found the APIs:
When does not find the APIs:
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments
(seechangelog/fragments/00-template.yaml
)website/content/en/docs