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

[Breadth Coverage] Log Analytics Solution #1545

Merged
merged 5 commits into from
Apr 29, 2020
Merged

Conversation

zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented Apr 15, 2020

Close: Azure/azure-cli#12706


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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@azuresdkci
Copy link

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/zhoxing-ms/azure-cli-extensions.git@dev#subdirectory=src/$EXT&egg=$EXT"

@zhoxing-ms zhoxing-ms force-pushed the dev branch 4 times, most recently from 99c8fda to a255492 Compare April 15, 2020 07:14
@yonzhan yonzhan added this to the S168 milestone Apr 15, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Apr 15, 2020

add to S168

{
"azext.isExperimental": true,
"azext.minCliCoreVersion": "2.0.67",
"azext.maxCliCoreVersion": "3.0.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Please don't set maxCliCoreVersion here
  • "azext.minCliCoreVersion" should be at least "2.3.0"; otherwise 'experimental' tag is not available. Please test if 'experimental' tag works first.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't set maxCliCoreVersion here
"azext.minCliCoreVersion" should be at least "2.3.0"

Done

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms Apr 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise 'experimental' tag is not available. Please test if 'experimental' tag works first.

Yeah, tag had been tested and can be used normally #Closed

Copy link
Member

@qianwens qianwens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

'Intended Audience :: Developers',
'Intended Audience :: System Administrators',
'Programming Language :: Python',
'Programming Language :: Python :: 2',
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove python 2, 2.7 as we will not support them any longer. #Closed

client_factory=cf_solutions, is_experimental=True) as g:
g.custom_command('create', 'create_monitor_log_analytics_solution')
g.custom_command('update', 'update_monitor_log_analytics_solution')
g.custom_command('delete', 'delete_monitor_log_analytics_solution')
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add 'confirmation=True' for delete command #Closed

# pylint: disable=unused-argument


def create_monitor_log_analytics_solution(cmd, client,
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameter cmd is not used, remove it. please also check not used cmd in other methods.

c.argument('plan_publisher', help='Publisher name of the plan for solution. For gallery solution, it is Microsoft.')
c.argument('plan_product', help='Product name of the plan for solution. '
'For Microsoft published gallery solution it should be in the format of OMSGallery/<solutionType>. This is case sensitive.')
c.argument('workspace_resource_id', options_list=['--workspace-resource-id', '-w'],
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please support workspace name as well #Closed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean use one argument "workspace" that supports both name and resource id as storage_account does.


In reply to: 415257345 [](ancestors = 415257345)

def load_arguments(self, _):

with self.argument_context('monitor log-analytics solution create') as c:
c.argument('resource_group', resource_group_name_type)
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary to add resource_group here as it's a global argument added by cli core automatically.
please also remove it in other methods. #Closed


with self.argument_context('monitor log-analytics solution create') as c:
c.argument('resource_group', resource_group_name_type)
c.argument('name', options_list=['--name', '-n'], help='Name of the log-analytics solution.')
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name [](start = 20, length = 4)

name [](start = 20, length = 4)

use solution_name here and define a CLIArgumentType for reuse #Closed



def create_monitor_log_analytics_solution(cmd, client,
resource_group,
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resource_group [](start = 42, length = 14)

resource_group [](start = 42, length = 14)

resource_group_name #Closed


def update_monitor_log_analytics_solution(cmd, client,
resource_group,
name,
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name [](start = 42, length = 4)

name [](start = 42, length = 4)

solution_name #Closed

if tags is not None:
body['tags'] = tags # dictionary

return client.create_or_update(resource_group_name=resource_group, solution_name=name, parameters=body)
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use update (which is a patch) instead. #Closed


with self.command_group('monitor log-analytics solution', log_analytics_solution_solutions,
client_factory=cf_solutions, is_experimental=True) as g:
g.custom_command('create', 'create_monitor_log_analytics_solution')
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add supports_no_wait=True for create/update/delete #Closed

text: |-
az monitor log-analytics solution create --resource-group MyResourceGroup \\
--name MySolutionGroup --location "East US" --tags key=value \\
--plan-name PlanName --plan-publisher PlanPublisher --plan-product PlanProduct \\
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the example be more specific for some arguments such a publisher, product? #Closed

def create_monitor_log_analytics_solution(cmd, client,
resource_group,
name,
location,
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

location=None to make it optional as it can be extracted from resource group. #Closed

name,
location,
workspace_resource_id,
plan_name,
Copy link
Member

@jsntcy jsntcy Apr 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plan_name is same as solution name? #Closed

@zhoxing-ms zhoxing-ms force-pushed the dev branch 2 times, most recently from 09f5681 to ee45bfb Compare April 27, 2020 07:27
@zhoxing-ms zhoxing-ms requested a review from YalinLi0312 as a code owner April 27, 2020 07:27
@zhoxing-ms zhoxing-ms force-pushed the dev branch 2 times, most recently from 3d4990f to c1101e9 Compare April 27, 2020 07:46
}
}

return client.create_or_update(resource_group_name=resource_group_name, solution_name=solution_name, parameters=body)
Copy link
Member

@jsntcy jsntcy Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrap with "sdk_no_wait", like "return sdk_no_wait(no_wait, client.create_or_update, resource_group_name, cluster_name, cluster_instance)". Please also update other commands that support no wait. #Closed

Comment on lines 12 to 15
helps['monitor log-analytics solution'] = """
type: group
short-summary: Commands to manage monitor log-analytics solution.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to indent the help. Ctrl+A to select all, then press Shift+Tab to remove the indentations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it~

Copy link
Member

@jsntcy jsntcy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

if not is_valid_resource_id(namespace.workspace_resource_id):
raise CLIError('usage error: --workspace-resource-id is invalid')
raise CLIError('usage error: --workspace is invalid, it must be name of resource id of workspace')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or

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 this pull request may close these issues.

[Breadth Coverage] Onboard OperationsManagement Commands
6 participants