Skip to content
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

Remove UAMI #76

Closed
majguo opened this issue Nov 13, 2022 · 7 comments · Fixed by #78
Closed

Remove UAMI #76

majguo opened this issue Nov 13, 2022 · 7 comments · Fixed by #78

Comments

@majguo
Copy link
Collaborator

majguo commented Nov 13, 2022

Description

To improve the usage of liberty-on-aro offer, @edburns initiated discussion with Azure Red Hat OpenShift team and they proposed the removal of uami which requires both permissions in subscription and Azure AD. Accordingly, user just needs to provide a service principal clientId and clientSecret which can easily be retrieved from the output of az ad sp create-for-rbac --sdk-auth command execution. We expect that this new approach would reduce friction and lead to an increase in ARO deployments.

Preview UI changes

Reviewer can preview the UI changes using the Create UI Definition Sandbox:

  • Open Create UI Definition Sandbox in a new tab of browser.
  • Open UI source code createUiDefinition.json in a new tab of the browser.
  • Replace the content of work area in Create UI Definition Sandbox with the content of UI source code createUiDefinition.json.
  • Click Preview at the left-bottom of Create UI Definition Sandbox.
  • The UI changes include:
    • Removed uami section from Basics tab. Added an information box to clarify the required roles for user who wants to deploy the solution template offer.
    • Added service principal section in Configure cluster tab.

Screenshots

Old UI:
image

New UI:
image

@gcharters
Copy link
Member

gcharters commented Nov 14, 2022

Hi @majguo I tried the command to create the service principal and got the following:

az ad sp create-for-rbac --sdk-auth
The underlying Active Directory Graph API will be replaced by Microsoft Graph API in Azure CLI 2.37.0. Please carefully review all breaking changes introduced during this migration: https://docs.microsoft.com/cli/azure/microsoft-graph-migration
Option '--sdk-auth' has been deprecated and will be removed in a future release.
Insufficient privileges to complete the operation.

With my account, I was previously able to create a UAMI, so whilst this option looks more straightforward, I'm wondering if fewer people will be able to do it? Also, are we able to replace --sdk-auth with something that isn't deprecated? I see there's some debate about it here - Azure/azure-cli#20743

@majguo
Copy link
Collaborator Author

majguo commented Nov 14, 2022

@gcharters and me have discussed the details and finally Gram agreed it's an improvement, details pls see https://ibm-cloud.slack.com/archives/G010BB252NA/p1668322984576439.

@edburns
Copy link
Collaborator

edburns commented Nov 15, 2022

@m_reza_rahman and I reviewed the UI with @majguo today. Plan of record is for Jianguo to create a PR and Ed will apply some copyediting to the UI text, but it is essentially ok as is.

@majguo majguo mentioned this issue Nov 16, 2022
@helyarp
Copy link

helyarp commented Nov 18, 2022

@majguo Hi, I reviewed the strings. Changes to the Basics tab look good. No suggested changes for the Basics tab.

I suggest several changes for the Configure cluster tab:

  • A "the" is missing from the second sentence. Maybe change "with command az ad sp create-for-rbac --sdk-auth." to "with the az ad sp create-for-rbac --sdk-auth command." (or otherwise add a "the")
  • Rather than use the phrase "From the output of the command execution," use a full sentence, such as "Use output from the command for service principal values." or "Copy command output into text boxes."

    Do Azure UIs typically refer to the fields as "text boxes"? I think the style is to use the label names rather than say "text box" or "button".

  • Maybe shorten the bulleted lists and, if Azure style is to start sentences with a capital letter, start the items with a capital letter. Suggested changes:
    • "Use the clientID value for Service principal client ID." or "Copy the clientID value and paste it into the Service principal client ID text box."
    • "Use the clientSecret value for Service principal client secret." or "Copy the clientSecret value and paste it into the Service principal client secret text box."

@majguo
Copy link
Collaborator Author

majguo commented Nov 19, 2022

Hello @helyarp Thanks for your review. The text strings have already been updated per review comments from @edburns and @m-reza-rahman. So may I ask you review it again? Sorry for the inconvenience caused.

image

Notes

  • We add "An Azure Active Directory service principal is required for cluster creation. For more information on creating a service principal, consult the Azure Red Hat OpenShift product documentation." at the beginning, which is copied from Create Azure Red Hat OpenShift Cluster > Authentication > Information box. The reason is that we adopt the similar design that asking a service principal from users and we want to keep the similar text as well.
  • Additionally, we have a second section with instructions for user about creating the service principal using a one-line command for the convenience, this is also reviewed and approved by @gcharters.

@helyarp
Copy link

helyarp commented Nov 28, 2022

@majguo Hi, please remove the "below"; "below" is an accessibility violation. Is there any way to run the command and have the values put into the fields automatically? As to two "Confirm secret" labels, I suppose it's okay; we try to have unique labels but the info note divides the similar labels.

@majguo
Copy link
Collaborator Author

majguo commented Nov 29, 2022

@helyarp Thanks for your review, here is my response.

Hi, please remove the "below"; "below" is an accessibility violation.

Is there any way to run the command and have the values put into the fields automatically?

  • Unfortunately, we have no way to automatically have the values put into the fields.

As to two "Confirm secret" labels, I suppose it's okay; we try to have unique labels but the info note divides the similar labels.

  • OK, I'll keep as it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants