Change flag workspace-resource-id to workspace-resource #11241

troy0820 · 2019-11-14T22:38:57Z

Issue #6949 (comment) describes this problem across AKS and Azure Red Hat Openshift when trying to use monitoring when creating clusters. To clearly distinguish the parameter that Azure Red Hat Openshift needs to create a cluster with monitoring, changing this parameter to workspace-resource and providing the context in the help menu when using az openshift create -h will allow the user to see what is expected. The conversation was in PR #10775 and settled on the former but subsequent work will create issues of handling the id with future commands.

Change help and parameters to match "workspace-resource"

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

troy0820 · 2019-11-15T01:58:29Z

@myronfanqiu Can you take a look at this?

mmyyrroonn · 2019-11-15T02:03:02Z

@troy0820 Hello. If you want to do this, you should support "Name of workspace". Actually I'm glad to see this PR. 😄 BTW, workspace_resource is a strange word.

olga-mir · 2019-11-17T22:21:29Z

@troy0820 @myronfanqiu what do you think abut using only --workspace?
According to guidelines: Arguments that end in -id should be GUIDs.. I believe the id part of this name is what causing the majority of confusion among users, it invites to put the "id"-like value. In the Azure Monitor UI what you will see is "Workspace Resource id" field with a guid value, which doesn't help to either.
Have a look at this issue and particularly this comment: #6949 (comment)

mmyyrroonn · 2019-11-18T01:52:14Z

@olga-mir @troy0820 Hi. Like @tjprescott said in the #6949 (comment), the main reason behind this guideline is that " The main reason is that something which accepts an ARM resource ID should typically be following the "name or ID" convention in the CLI."
--workspace is a better argument for user if it supports both ID or Name of the workspace. We don't need to create a default workspace for user. If user gives the name of the workpsace, we usually assume that this workspace is under the same resource group with openshift.

olga-mir · 2019-11-18T02:32:03Z

thanks for clarifying this @myronfanqiu, I can see your point, you can read this comment in both ways. If most people agree on this name then it's fine. @troy0820 did provide help text, so hopefully that helps to resolve confusion.

olga-mir · 2019-11-18T02:39:29Z

src/azure-cli/azure/cli/command_modules/acs/_help.py

nit: would it be better to use MyResourceGroup or MyRG style instead of <resourcegroup>? Other help text strings that include ARM ID don't use <>
(git grep "00000000-0000-0000-0000-000000000000" | grep -v tests)

mmyyrroonn · 2019-11-20T01:39:28Z

@troy0820 Hello. Any progress on this PR? I remembered the GA date is around today.

troy0820 · 2019-11-20T02:14:20Z

@MyronFanQui I can fix the nit that @olga-mir pointed out. I didn't update the PR to support both the name of the workspace. I can do that and have it hopefully land tomorrow.

troy0820 · 2019-11-26T15:12:36Z

@myronfanqiu I've updated the PR to accept the string that is just the workspace-id. If the string is just that, it will create the "/subscription/...." necessary to provide the monitor profile for az openshift create. If the user inputs the "/subscription/..." it will default to that and create the monitor profile. Can you take a look?

mmyyrroonn

Could we add a single test for this command?

mmyyrroonn · 2019-11-27T05:11:43Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

workspace_id=workspace_id instead of workspace_resource_id=workspace_id?

If the string passed in to --workspace-id is, in fact, jus the name of the workspace-id, the variable will still be workspace-id and reference the string to build the workspace-resource-id (/subscriptions/...).

mmyyrroonn · 2019-11-27T05:11:56Z

src/azure-cli/azure/cli/command_modules/acs/custom.py

Is it possible for server end?

@JackQuincy mentioned in the other PR that this has to be done client side. AKS does the same validation for their workspace-resource-id on line 2204.

The regex validation was checking the subscription but that is wrong. Changing it to append the name of the workspace resource id to the string to build the monitor profile.

Can you add a test for this command? It's a little bit strange to have a id like '/subscriptions/{}/resourceGroups/{}/providers/Microsoft.OperationalInsights/workspaces/{}'.format(subscription, resource_group_name, workspace_id) The workspace_id is something like xxxx-xxxx-xxxx-xxxx-xxxx-xxxx which is strange.

@myronfanqiu I'm confused. When you do az resource list the id of the loganalytics workspace is this string:

/subscriptions/{subscription}/resourceGroups/{resourceGroup}/providers/Microsoft.OperationalInsights/workspaces/{workspace-name}

The workspace name is the just 4-63 characters after /workspaces/. The logic allows for either the string to be passed in with the workspace name as the id or just the workspace name which builds the id because the OpenshiftMonitorProfile needs the whole id to create the managed cluster.

I will indeed create a test for the functionality of the above logic.

The id I believe you are describing is the subscription id that is passed with the id of the loganalytics workspace id. Unless I'm mistaken.

@troy0820 Oh. Since the name of the argument is workspace-id, I thought this command supported both workspace_resource_id and workspace_id. If you are intend to support Name or Resource ID. Usually, the name of the argument should be just workspace and specify the detail information in the help message. We still need a test for this command. Thanks~

I will add a test for this but will also wait until this PR #11460 is merged and I can rebase. It changes the imports to not obscure the API version from the caller and only use the preview 0930 imports.

Change help and parameters to match "workspace-resource"

…ift create

mmyyrroonn · 2019-12-10T05:02:24Z

src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_osa_commands.py

Is it a workspace name instead of workspace id?

It is a workspace resource id. The workspace name and id are 2 other fields that exist and won't work here. We are using the example inputs to try to make it clear what kind of id it is.

This is the workspace resource id as Jack said. We are using the same mechanism that we use to force users to grab this id as we do for VNET peering listed in the example inputs. Trying to mock this means I need a subscription. I can test that the monitor profile is indeed None when creating a cluster. The cluster will provision as before but with a blank monitoring profile.

We also do not provide a default log analytics workspace if none is provided like AKS does with its add-on capabilities.

Looks like you delete the logic and it's just a renaming PR. Right?

I deleted the logic because as of right now we do not want to support workspace name for the reasons @JackQuincy explained. I am just using this PR to rename the parameters around the flag for documentation purposes. The subsequent work in the future should address these issues. The user of this flag will be demonstrated within the examples and forced to used the existing workspace in the format that is similar to VNET peering.

[openshift]Test command for openshift create with monitor profile as None

mmyyrroonn · 2019-12-11T01:41:23Z

src/azure-cli/azure/cli/command_modules/acs/tests/latest/test_osa_commands.py

+        })
+
+        # create
+        create_cmd = 'openshift create --resource-group={resource_group} --name={name} --location={location} ' \


is it able to link a workspace since this PR is related with workspace? We have az monitor log-analytics workspace create

mmyyrroonn · 2019-12-12T02:07:36Z

@troy0820 Actually, this should be a breaking change for users. However, I just checked usage data and it's quite low. So I think it's ok.

mmyyrroonn · 2019-12-12T02:11:10Z

@troy0820 Could you add [Breaking Change] in History.rst? Or I can add it for you later in other PR if you don't mind.

…t create command

troy0820 · 2019-12-12T02:41:25Z

@myronfanqiu I don't mind if you add it. I think that'll work if that is easier.

mmyyrroonn · 2019-12-12T05:14:42Z

@troy0820 to resolve conflicts in history, I added history in another PR #11522

yonzhan assigned mmyyrroonn Nov 16, 2019

yonzhan requested a review from mmyyrroonn November 16, 2019 14:47

yonzhan added this to the S161 milestone Nov 16, 2019

olga-mir reviewed Nov 18, 2019

View reviewed changes

mmyyrroonn suggested changes Nov 27, 2019

View reviewed changes

yonzhan modified the milestones: S161, S162 Dec 1, 2019

yonzhan requested a review from haroldrandom December 1, 2019 04:52

troy0820 added 3 commits December 5, 2019 09:48

[openshift]Change "--workspace-resource-id" to "workspace-resource"

d98929b

Change help and parameters to match "workspace-resource"

[openshift]Change HISTORY to reflect change in parameter in az opensh…

dd74388

…ift create

[openshift]Change paramter to workspace-id

74d8fbe

troy0820 force-pushed the openshift/workspace-resource branch 2 times, most recently from 660aa93 to 92b4366 Compare December 5, 2019 14:49

troy0820 requested a review from mmyyrroonn December 9, 2019 15:06

mmyyrroonn reviewed Dec 10, 2019

View reviewed changes

[openshift]Change workspace_resource_id to workspace_id

2970cbe

[openshift]Test command for openshift create with monitor profile as None

troy0820 force-pushed the openshift/workspace-resource branch from 62b9190 to 2970cbe Compare December 10, 2019 18:00

mmyyrroonn reviewed Dec 11, 2019

View reviewed changes

mmyyrroonn approved these changes Dec 12, 2019

View reviewed changes

[openshift]Add workspace_id to test for monitorProfile in az openshif…

91d7c20

…t create command

troy0820 force-pushed the openshift/workspace-resource branch from d8de3f6 to 91d7c20 Compare December 12, 2019 02:38

MyronFanQiu added 2 commits December 12, 2019 12:54

refine History

d444557

remove history and add it in another PR to solve conflicts

25b8edb

mmyyrroonn merged commit addbaf4 into Azure:dev Dec 12, 2019

olga-mir mentioned this pull request Jan 20, 2020

[Azure Red Hat OpenShift] Add monitor subgroup to manage Log Analytics monitoring in Azure Red Hat OpensShift cluster #11904

Merged

2 tasks

Change flag workspace-resource-id to workspace-resource #11241

Change flag workspace-resource-id to workspace-resource #11241

Uh oh!

Conversation

troy0820 commented Nov 14, 2019

Uh oh!

troy0820 commented Nov 15, 2019

Uh oh!

mmyyrroonn commented Nov 15, 2019

Uh oh!

olga-mir commented Nov 17, 2019

Uh oh!

mmyyrroonn commented Nov 18, 2019

Uh oh!

olga-mir commented Nov 18, 2019

Uh oh!

olga-mir Nov 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmyyrroonn commented Nov 20, 2019

Uh oh!

troy0820 commented Nov 20, 2019

Uh oh!

troy0820 commented Nov 26, 2019

Uh oh!

mmyyrroonn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmyyrroonn commented Dec 12, 2019

Uh oh!

mmyyrroonn commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

troy0820 commented Dec 12, 2019

Uh oh!

mmyyrroonn commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

olga-mir Nov 18, 2019 •

edited

Loading

mmyyrroonn commented Dec 12, 2019 •

edited

Loading