-
Notifications
You must be signed in to change notification settings - Fork 417
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 #3707
Bump CAPI to v1.5.0 #3707
Conversation
/retitle [WIP] Bump CAPI to v1.5.0-rc.0 |
I have a handle on fixing the remaining errors related to controller-runtime, except for this one: # github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601
../../../pkg/mod/github.com/!azure/azure-service-operator/v2@v2.1.0/api/resources/v1api20200601/resource_group_types_gen.go:178:29: cannot use &ResourceGroup{} (value of type *ResourceGroup) as admission.Validator value in variable declaration: *ResourceGroup does not implement admission.Validator (wrong type for method ValidateCreate)
have ValidateCreate() error
want ValidateCreate() (admission.Warnings, error)
make: *** [Makefile:198: manager] Error 1 @nojnhuh does this imply we need a new release of ASO? |
Yes, that's what it looks like to me. ASO is currently on controller-runtime 0.13: |
A temporary alternative to waiting for an ASO release would be ripping out all of our ASO resource group stuff. |
I opened a request for controller-runtime v0.15.0 support at Azure/azure-service-operator#3137. Hopefully they're ok with upgrading–it is a breaking change. |
31a70fa
to
3ff5b9d
Compare
@mboersma Yeah that's unfortunate that the versions of controller runtime don't line up. Is there anything we can do for that besides wait for them to release a new version after upgrading to v0.15.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.
LGTM pending the ASO issue
We could back out the current ASO implementation for resource groups if need be, but that wouldn't really change anything, just buy some more time. Maybe we could make an adaptor-type struct that would override |
3ff5b9d
to
6280f45
Compare
Sounds good. Do you have an idea of when we want to get this PR in? We would need to get this in order to merge #3736 as it requires bumping to v1.5 as well. |
As soon as we can after the CAPI release, as always (planned for July 25), but we will also need to get a new ASO release and we don't control that timeline. The good news is that team pounced on our request and already have a PR in review that upgrades controller-runtime to match CAPI 1.5, so hopefully an ASO release isn't far away. |
6280f45
to
8f788cc
Compare
@mboersma Here are the changes to include ASO v2.2.0 which includes controller-runtime 0.15: 28b88459 It looks like ASO's CRD manifest and docker image for the new release aren't out yet, but I've asked about that in Slack and did a quick smoke test with locally-built versions and I expect everything to work once things are published. |
ASO release kinks appear to have been worked out. |
8f788cc
to
927413b
Compare
/retest |
@Jont828 AFAICT it's ready to go, but we've had several flakes in the main e2e test, including the one I noted above. That test hasn't been that reliable, and the flakes seem to be similar to ones on other PRs. My only concern is that perhaps v1.5.0 is introducing some further instability there, but it's really hard to discern. I haven't been able to reproduce any of these failures, nor track down any specifics through the logs. They don't seem to be related to this PR, but I can't say so for sure. |
Sounds good, I'm comfortable moving this forward if we're seeing similar flakes in other PRs. |
225099f
to
b01b803
Compare
Rebased due to a collision in test code from #3826. |
I looked at the e2e test history and I don't see a pattern in the failures |
b01b803
to
2dd24fc
Compare
Rebased again due to collision in go.sum with #3827. |
/lgtm |
LGTM label has been added. Git tree hash: 93fa01bf8fc44fc51c13bc358dfa977ef4f6e75b
|
/test pull-cluster-api-provider-azure-e2e |
/test pull-cluster-api-provider-azure-e2e-optional |
@@ -68,15 +69,15 @@ var ( | |||
) | |||
|
|||
// validateCluster validates a cluster. | |||
func (c *AzureCluster) validateCluster(old *AzureCluster) error { | |||
func (c *AzureCluster) validateCluster(old *AzureCluster) (admission.Warnings, error) { |
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 really cool! we should use those, there are a few places where I'm sure they'd be useful
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
/approve
/hold
feel free to remove hold when optional tests have passed
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon 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 |
/hold cancel |
/override pull-cluster-api-provider-azure-apidiff |
@mboersma: Overrode contexts on behalf of mboersma: pull-cluster-api-provider-azure-apidiff 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. |
We made it! |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Updates CAPI to v1.5.0 and all that entails.
Which issue(s) this PR fixes:
Fixes #3680
Closes #3751
Closes #3773
Special notes for your reviewer:
TODOs:
Release note: