-
Notifications
You must be signed in to change notification settings - Fork 33
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
⚠️ Bump CAPI to v1.5.0, controller-runtime to v0.15.0 and controller-tools to v0.12.1 #280
⚠️ Bump CAPI to v1.5.0, controller-runtime to v0.15.0 and controller-tools to v0.12.1 #280
Conversation
7469130
to
f1374fe
Compare
/test gomod |
/retest |
}, | ||
expectedAllocations: map[string]ipamv1.IPAddressStr{}, | ||
expectedNbAllocations: 0, | ||
}), |
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.
what is this change related to?
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.
Due to CR v0.15.0 changes, fake client will panic if the object is initialized with deleteionTimestamp without Finalizer,
Name: "abcpref-192-168-1-11", | ||
Namespace: "myns", | ||
}, | ||
}, |
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.
And this?
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.
When IpAddress created, it will update ipClaimStatus firtst and then will proceed with ip address creation. Here somehow we did not include status in IpClaim which is causing an error in current testcase. Not sure how it was passing before.
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.
Interesting, it shouldn't have passed if thats the case.
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.
It was actually due to status handling changes in CR v0,15.0. Fixed now
f1374fe
to
466b2b2
Compare
36acb9a
to
a007085
Compare
@Sunnatillo: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed 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.
You need to bump controller-runtime in api/go.mod as well to v0.15.0
a007085
to
75c0613
Compare
75c0613
to
fd4fc3a
Compare
/test-centos-e2e-integration-main |
/test-ubuntu-integration-main |
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.
Very good job. Few small nits inline. This PR is in pretty good shape.
- stylecheck | ||
- thelper | ||
- typecheck | ||
- unconvert | ||
- unused | ||
- varcheck |
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.
Can you please add some explanation about removing these checks in the PR description?
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 | |||
kind: CustomResourceDefinition | |||
metadata: | |||
annotations: | |||
controller-gen.kubebuilder.io/version: v0.11.4 | |||
controller-gen.kubebuilder.io/version: v0.12.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.
CAPI is sitting currently on v0.12.0. Its fine for now since its a patch version but lets wait for CAPI to bump this for future bumps.
hack/tools/go.mod
Outdated
@@ -4,8 +4,8 @@ go 1.20 | |||
|
|||
require ( | |||
github.com/jteeuwen/go-bindata v3.0.7+incompatible | |||
k8s.io/code-generator v0.26.1 | |||
sigs.k8s.io/controller-tools v0.11.4 | |||
k8s.io/code-generator v0.27.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.
you have bumped other k8s packages to v0.27.2 in root go.mod and api/go.mod. Can you check if we can do the same for this package.
Name: "abcpref-192-168-1-11", | ||
Namespace: "myns", | ||
}, | ||
}, |
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.
Interesting, it shouldn't have passed if thats the case.
/retitle Bump CAPI to v1.5.0, controller-runtime to v0.15.0 and controller-tools to v0.12.1 |
fd4fc3a
to
c81b271
Compare
/test-centos-e2e-integration-main |
/cc @smoshiur1237 |
/hold |
ipam/ippool_manager_test.go
Outdated
@@ -416,7 +422,7 @@ var _ = Describe("IPPool manager", func() { | |||
}, | |||
Status: ipamv1.IPClaimStatus{ | |||
Address: &corev1.ObjectReference{ | |||
Name: "abcpref-192-168-1-12", | |||
Name: "abcpref-192-168-1-10", |
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.
Would you please check if this is correct? I think it doesn't need 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.
yes, thank you for noticing. Fixed it.
remove deprecated linters(varcheck, structcheck, deadcode)
c81b271
to
cbaa4b4
Compare
/test-centos-e2e-integration-main |
/unhold |
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 for the fix.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kashifest 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 |
/retitle |
This PR:
WithStatusSubresource
to fake client, due to status handling changes in CR v0.15.0.