-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
61a2f55
to
140a2b0
Compare
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 the contribution 🥇
See the comment: https://github.com/kubernetes-sigs/kubebuilder/pull/2679/files#r871211532
140a2b0
to
b17731d
Compare
@camilamacedo86 Unit tests are failing All error logs are like:
|
This PR is blocked by upgrading the declarative project: kubernetes-sigs/kubebuilder-declarative-pattern#217. PS.: The first version of go/v3 uses controller-runtime v0.7.2 ( see: https://github.com/kubernetes-sigs/kubebuilder/blob/v3.0.0/testdata/project-v3/go.mod#L12 ), so the changes described in kubernetes-sigs/controller-runtime#1903 is fine. However, we will need to update the migration guide doc from v2 to v3 with: kubernetes-sigs/controller-runtime#1903 |
@NikhilSharmaWe these errors seem to be related to the kubebuilder-declarative-pattern dependency, so since your PR to bump it to support k8s 1.24 was merged, I think this line needs to be modified:
to be: kbDeclarativePatternForV3 = "fe5be9431eae158f86f9de23000a9a2ec06745fc" |
b17731d
to
9214963
Compare
aba99f8
to
5528cde
Compare
5528cde
to
521985a
Compare
@NikhilSharmaWe I think I may have found the problem that is causing the tests to fail with the proto file issue. ProblemThe problem seems to be because the go.mod dependencies are being populated with indirect dependencies that map to the k8s packages with a version of The exact indirect dependency that is causing the issue is SolutionThe solution I found is to modify the // Due to some indirect dependency changes with a bump in k8s packages from 0.23.x --> 0.24.x we need to
// clear out all indirect dependencies before we run `go mod tidy` so that the above changes get resolved correctly
if err := util.ReplaceRegexInFile("go.mod", `(require \(\n(\t.* \/\/ indirect\n)+\))`, ""); err != nil {
log.Warnf("unable to update the go.mod indirect dependencies: %s", err)
} I will also leave a PR review comment around the area in the file it should be added. |
@@ -145,7 +145,7 @@ manifests: controller-gen` | |||
} | |||
|
|||
if err := util.ReplaceInFile("go.mod", | |||
"k8s.io/klog/v2 v2.30.0", | |||
"k8s.io/klog/v2 v2.60.1", | |||
"k8s.io/klog/v2 v2.9.0"); err != nil { | |||
log.Warnf("unable to update the go.mod with k8s.io/klog/v2 v2.9.0: %s", err) | |||
} |
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.
After this line we should add:
// Due to some indirect dependency changes with a bump in k8s packages from 0.23.x --> 0.24.x we need to
// clear out all indirect dependencies before we run `go mod tidy` so that the above changes get resolved correctly
if err := util.ReplaceRegexInFile("go.mod", `(require \(\n(\t.* \/\/ indirect\n)+\))`, ""); err != nil {
log.Warnf("unable to update the go.mod indirect dependencies: %s", err)
}
The reasoning for this change can be seen here: #2679 (comment)
go.mod
Outdated
sigs.k8s.io/controller-runtime v0.11.2 | ||
golang.org/x/tools v0.1.10-0.20220218145154-897bd77cd717 | ||
k8s.io/apimachinery v0.24.0 // for `kubebuilder alpha config-gen` | ||
sigs.k8s.io/controller-runtime v0.12.1 | ||
sigs.k8s.io/controller-tools v0.8.0 // for `kubebuilder alpha config-gen` |
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.
controller-tools should be upgraded to v0.9.0
since it doesn't support k8s 1.24 until that release. More info on the v0.9.0 release can be found here: https://github.com/kubernetes-sigs/controller-tools/releases/tag/v0.9.0
sigs.k8s.io/controller-tools v0.8.0 // for `kubebuilder alpha config-gen` | |
sigs.k8s.io/controller-tools v0.9.0 // for `kubebuilder alpha config-gen` |
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.
@NikhilSharmaWe,
We need also add this info in the title and description
I am doing tha
521985a
to
1421ff9
Compare
@NikhilSharmaWe it looks like the new unit test failures are related to an update made in controller-tools v0.9.0 that no longer writes the Due to this we need to update the expected files to match the new format. To do this you need to:
After that the unit tests should pass again. |
1421ff9
to
4b37fe7
Compare
…om v0.11.2 to v0.12.1
4b37fe7
to
98e8df7
Compare
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.
Looks good to me, great job @NikhilSharmaWe !
/cc @joelanford Could you take a look at this so we can try and get this in ASAP? Thanks! |
What does this error mean?
|
That seems to be because of an APIDiff check - modifying an exported field. If someone with permissions could override that check, we can merge it. |
@jmrodri from what I can tell it seems to just be stating that the bump to v0.12.1 is a breaking change from v0.11.2 which we already knew. As @varshaprasad96 mentioned, I think this can be safely overridden. |
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
@jmrodri: changing LGTM is restricted to collaborators In response to this:
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. |
Looks pretty straight forward to me. |
kind: "" | ||
plural: "" | ||
conditions: [] | ||
storedVersions: [] |
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.0
// clear out all indirect dependencies before we run `go mod tidy` so that the above changes get resolved correctly | ||
if err := util.ReplaceRegexInFile("go.mod", `(require \(\n(\t.* \/\/ indirect\n)+\))`, ""); err != nil { | ||
log.Warnf("unable to update the go.mod indirect dependencies: %s", err) | ||
} |
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
- The indirect dependencies in the
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. - As a result of the above, the indirect dependencies that are being used may be incompatible with the versions that the direct dependencies are using. I think we should be ensuring that the indirect dependencies that are being used are the same as those that are used when manually creating a new
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.
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
/approved
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, everettraven, jmrodri, NikhilSharmaWe 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 |
Closes: #2681