-
Notifications
You must be signed in to change notification settings - Fork 176
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
Fix MIWI admin update #4046
Fix MIWI admin update #4046
Conversation
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.
Change LGTM as-is, but some follow-up questions/actions:
- After each of the admin updates runs, did an admin GET and verified that all MIWI-specific fields were present in cluster doc and available via the admin API (except for the identity URL and the bound service account signing key, which I don't think we need to be there)
Are the identity URL and boundSASigningKey still present on the clusterdoc in Cosmos and just not displayed on the admin API, or are they actually wiped from the database? Even if we don't technically need these values after a successful install, I think we should try to preserve them in the database in case there's something in the future that requires their use, but we absolutely should not be exposing them on the admin API.
Note that the Jira says we should add unit tests to ensure that none of the MIWI fields are clobbered/erased. It can't really be effectively unit tested. We could add it to MIWI E2E tests though - it would be very easy to check there.
They're a little cumbersome to write (apologies), but the frontend's putorpatch test would be the place to add unit tests: https://github.com/Azure/ARO-RP/blob/master/pkg/frontend/openshiftcluster_putorpatch_test.go. These are our "these put/patch payloads result in this database result and responses" unit tests as far as I'm aware. I'd be fine with these tests added in a follow-up PR though.
They're preserved in CosmosDB but not included in the admin API response.
Oh, right. I can fit those unit tests into this PR! |
I'm going to opt to merge this now so it makes it in our release - we can handle this in a followup PR. |
* Fix admin API - add readOnly tag to two fields that need it * Fix admin update - if cluster is MIWI, skip step specific to service principal clusters * Fix admin update - ensure that MIWI cluster doc fields are not overwritten * Remove extra whitespace to appease linter
Which issue this PR addresses:
https://issues.redhat.com/browse/ARO-12033
What this PR does / why we need it:
This PR fixes several small issues with admin updates on MIWI clusters. Each one of my original three commits to this branch fixes one issue.
Test plan for issue:
Note that the Jira says we should add unit tests to ensure that none of the MIWI fields are clobbered/erased. It can't really be effectively unit tested. We could add it to MIWI E2E tests though - it would be very easy to check there.
Is there any documentation that needs to be updated for this PR?
N/A
How do you know this will function as expected in production?
Will be tested in canary before the MIWI feature goes live.