-
Notifications
You must be signed in to change notification settings - Fork 425
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
WIP: Update docs on updating clusterctl API version upgrade tests #4996
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Update the [API version upgrade tests](https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/v1.12.1/test/e2e/capi_test.go#L214) to use the oldest supported release versions of CAPI and CAPZ after the release is cut as "Init" provider versions. See [this PR](https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/4433) for more details. | ||
Update the provider versions for the [API version upgrade tests](https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/v1.16.0/test/e2e/common.go#L94-L95) to use the latest patch release of the previous two minor releases. See [this PR](https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/4873) for more details. | ||
|
||
The two versioned prow templates within `test/e2e/data/infrastructure-azure` should be updated to `templates/test/ci/cluster-template-prow.yaml` at the respective version. For example, if we're updating `v1.14.4` to `v1.14.5`, then `cluster-template-prow.yaml` should be moved from `test/e2e/data/infrastructure-azure/v1.14.4` to `test/e2e/data/infrastructure-azure/v1.14.5`, and contain the same contents as `templates/test/ci/cluster-template-prow.yaml`. |
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.
I don't think this describes the nuance that the template in test/e2e/data/infrastructure-azure/v*
needs to keep its AzureClusterIdentity and not use the one from templates/test/ci/cluster-template-prow.yaml
. Instead of updating this doc for now, I'd rather see if we can get kustomize to start generating these templates and automatically take care of that. Explaining that in plain language here is going to make this too cumbersome IMO.
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.
Sure, I can pivot this PR to get kustomize to generate it, since leaving these docs are outdated as-is. So the template is templates/test/ci/cluster-template-prow.yaml
but with the AzureClusterIdentity from test/e2e/data/infrastructure-azure/v*
?
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 formula we'd want is templates/test/ci/cluster-template-prow.yaml + this patch:
diff --git a/test/e2e/data/infrastructure-azure/v1.15.1/cluster-template-prow.yaml b/test/e2e/data/infrastructure-azure/v1.15.1/cluster-template-prow.yaml
index abcfffe4f..af51d864c 100644
--- a/test/e2e/data/infrastructure-azure/v1.15.1/cluster-template-prow.yaml
+++ b/test/e2e/data/infrastructure-azure/v1.15.1/cluster-template-prow.yaml
@@ -381,9 +381,10 @@ metadata:
namespace: default
spec:
allowedNamespaces: {}
- clientID: ${AZURE_CLIENT_ID_USER_ASSIGNED_IDENTITY}
+ clientID: ${AZURE_CLIENT_ID_CLOUD_PROVIDER}
+ resourceID: /subscriptions/${AZURE_SUBSCRIPTION_ID}/resourceGroups/${CI_RG:=capz-ci}/providers/Microsoft.ManagedIdentity/userAssignedIdentities/${USER_IDENTITY:=cloud-provider-user-identity}
tenantID: ${AZURE_TENANT_ID}
- type: WorkloadIdentity
+ type: UserAssignedMSI
---
apiVersion: addons.cluster.x-k8s.io/v1beta1
kind: ClusterResourceSet
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 isn't super critical though, so maybe opening an issue for now is best while we iron out the rest of the failing tests.
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.
Created an issue #4997. Should I close this PR or just include the changes on line 204 to the docs so that it points to the correct files?
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.
Let's stand by to see how things shake out in #4992
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4996 +/- ##
=======================================
Coverage 51.18% 51.18%
=======================================
Files 274 274
Lines 24625 24625
=======================================
Hits 12605 12605
Misses 11234 11234
Partials 786 786 ☔ View full report in Codecov by Sentry. |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
This PR updates documentation on how to update the clusterctl API version upgrade tests on a new release. Previously the docs were pointing to the wrong place for updating versions, as well as not including guidance on how to update the
cluster-template-prow.yaml
that the test was using.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
Release note: