ARO-6425 v20240812preview validation #3563
Conversation
yithian
requested review from
jewzaam,
bennerv,
hawkowl,
rogbas,
petrkotas,
jharrington22,
cblecker,
cadenmarchese,
UlrichSchlueter,
s-amann,
SudoBrendan,
Shivkumar13,
yjst2012,
jaitaiwan,
anshulvermapatel,
hlipsig and
tiguelu
as code owners
yithian
changed the title
|
Please rebase pull request. |
This will enable OpenShiftClusterProperties.ServicePrincipalProfile to be set to nil
yithian
force-pushed
the
ARO-6425_v20240812preview_validation
branch
from
f4314dd
to
f7a880b
Compare
As these functions don't really fit into any of the util packages and are only used in one place each, this commit removes those functions and in-lines their code in the places they were called
yithian
force-pushed
the
ARO-6425_v20240812preview_validation
branch
from
4b95ad8
to
ca9536a
Compare
| PlatformWorkloadIdentities: []PlatformWorkloadIdentity{ | ||
| { | ||
| OperatorName: "FAKE-OPERATOR", | ||
| ResourceID: "/subscriptions/12345678-1234-1234-1234-123456789012/resourceGroups/a-fake-group/providers/Microsoft.RedHatOpenShift/userAssignedIdentities/fake-cluster-name", |
There was a problem hiding this comment.
should this be Microsoft.ManagedIdentity/userAssignedIdentities?
same for other test cases.
There was a problem hiding this comment.
The regexp the resourceID is being compared against is at pkg/api/v20240812preview/openshiftcluster_validatestatic.go line 430
The specific provider for the resource ID isn't being checked, just that the string is formatted like a resource ID at all. I can change the value in the test cases to Microsoft.ManagedIdentity if you'd like
… IDs This requires a refactor of ParseArmResourceId() to allow for resource IDs without a subresource
Co-authored-by: Kipp Morris <117932707+kimorris27@users.noreply.github.com>
"operator roles" becomes "platform workload identities"
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
kimorris27
dismissed
their stale review
Dismissing my own approving review because I realized I missed an open comment.
|
Thanks for being flexible and responding to all of the suggestions quickly. Will LGTM once Caden's last comment is addressed. |
|
/azp run ci,e2e |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Which issue this PR addresses:
For ARO-6425
What this PR does / why we need it:
Adds static validation for new API fields added in v20240812preview
Test plan for issue:
Includes unit tests for the new static validation features.
Is there any documentation that needs to be updated for this PR?
No, this is just static validation to ensure inputs are as expected.
How do you know this will function as expected in production?
Unit test data has been written to match expected inputs but nonetheless this should be tested in a lower environment before being pushed to production