Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
⚠️ (go/v3 and config-gen alpha command) Update controller-runtime dep from v0.11.2 to v0.12.1, controller-tools from v0.8.0 to v0.9.0 and k8s deps from 1.23 to 1.24.1 #2679
⚠️ (go/v3 and config-gen alpha command) Update controller-runtime dep from v0.11.2 to v0.12.1, controller-tools from v0.8.0 to v0.9.0 and k8s deps from 1.23 to 1.24.1 #2679
Changes from all commits
98e8df7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why it was removed?
Do we know that?
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 due to this PR: kubernetes-sigs/controller-tools#630
It changed the generation of a CRD to no longer include the status fields. This PR was included in the
v0.9.0
release of controller-tools. Here is the link to the release: https://github.com/kubernetes-sigs/controller-tools/releases/tag/v0.9.0There 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.
After this line we should add:
The reasoning for this change can be seen here: #2679 (comment)
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.
On top we are been very precisely about what dep we should downgrade
So, why do we need to add this one?
I think is better we just update what we scaffold by default instead of all it might change things that were not done in the default scaffold.
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 was a change that I recommended. I think it is fine if we want to change this to only include changing the dependency that was causing an issue, however I will explain my reasoning for the recommendation below.
Reasoning for recommended change
go.mod
seem to be getting populated with the indirect dependencies that correlate to the base scaffolding (e.g. k8s 1.24 dependencies) and are not properly updated when the direct dependency versions are downgraded. When I tested this locally, runninggo mod tidy
did not update the indirect dependencies when downgrading direct dependencies.go.mod
that contains the same direct dependencies. The best way I found to do this with Kubebuilder's scaffolding is to remove all the indirect dependencies and then rungo mod tidy
to repopulate them. This also helps to ensure that all the indirect dependencies will be compatible with the expectations of the downgraded direct dependencies.@camilamacedo86 WDYT? If this should be considered out of the scope of this PR and this is something we should still change, I am happy to create an issue to track this and just have this PR fix the dependencies it needs to fix.
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.
@everettraven I am ok with we better address it as you suggest in a follow up since it is only used with the deprecated flag --crd-version=v1beta1 which no longer works with k8s >= 1.22.