-
Notifications
You must be signed in to change notification settings - Fork 170
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
CLI Phase 1 - add UpgradeableTo field to update functionality #3844
CLI Phase 1 - add UpgradeableTo field to update functionality #3844
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.
Looks good! We just need to make sure we add unit tests for this and complete functional testing before merge.
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.
Added some change request comments regarding the functionality.
There are no comments now that impacts functionality, just some suggestions.
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 for the most part! I left one suggestion for additional validation, but I'd appreciate if someone else would verify that it's needed.
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 looks to me like we should also disallow the combination of refresh-credentials
with upgradeable-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.
I agree with Kipp's argument here, should we also disallow refresh-credentials
with enable_managed_identity
and platform_workload_identities
?
I would wait for @tsatam backing on this though.
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 we need to worry about enable_managed_identity
. It's not possible to specify it together with refresh_credentials
, since the former can only be passed to az aro create
and the latter can only be passed to az aro update
.
As for the other two suggestions in this thread:
- Disallow
refresh_cluster_credentials
together withupgradeable_to
- Disallow
refresh_cluster_credentials
together withplatform_workload_identities
@slawande2 could definitely do 1, but 2 is technically out of scope for this PR; we missed it when #3709 was in review. Maybe it's worth putting 2 in its own little PR?
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
7791400
Please rebase pull request. |
3f3e175
to
2fa2721
Compare
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
There's still an outstanding issue with the python lint - I think that this should fix it.
Running flake8 on extensions...
ERROR: /__w/1/go/src/github.com/Azure/ARO-RP/python/az/aro/azext_aro/tests/latest/unit/test_validators.py:822:7: W291 trailing whitespace
/__w/1/go/src/github.com/Azure/ARO-RP/python/az/aro/azext_aro/tests/latest/unit/test_validators.py:827:7: W291 trailing whitespace
/__w/1/go/src/github.com/Azure/ARO-RP/python/az/aro/build/lib/azext_aro/tests/latest/unit/test_validators.py:822:7: W291 trailing whitespace
/__w/1/go/src/github.com/Azure/ARO-RP/python/az/aro/build/lib/azext_aro/tests/latest/unit/test_validators.py:827:7: W291 trailing whitespace
4 W291 trailing whitespace
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
Which issue this PR addresses:
ARO-9607
What this PR does / why we need it:
Add support for Cx to provide the value of the OpenShift version they want to upgrade to using UpgradeableTo field during update such that the backend can validate platform workload identities for the current and upgradeableTo minor versions and federate the upgradeableTo version identities.
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
Not yet
How do you know this will function as expected in production?
Preview extension-only change. We will perform comprehensive testing before releasing this extension to users.