[amg] Azure managed grafana options for notification channels by michelletaal · Pull Request #5033 · Azure/azure-cli-extensions

michelletaal · 2022-06-22T14:36:52Z

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

Related command

General Guidelines

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

For new extensions:

[ x] My extension description/summary conforms to the Extension Summary Guidelines.

About Extension Publish

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

ghost · 2022-06-22T14:37:04Z

Thank you for your contribution michelletaal-shell! We will review the pull request and get back to you soon.

ghost · 2022-06-22T14:37:05Z

All CLA requirements met.

yonzhan · 2022-06-22T15:15:15Z

amg

yugangw-msft · 2022-06-23T18:42:43Z

@kairu-ms, can you please review this PR and merge appropriately?

yugangw-msft · 2022-06-23T18:47:49Z

src/amg/azext_amg/commands.py


+    with self.command_group('grafana notification-channel') as g:
+        g.custom_command('list', 'list_notification_channels')
+        g.custom_command('list-short', 'list_notification_channels_short')


I suggest we merge this with list command and expose an argument of --short

Valid suggestion. Applied in commit [REV1]

kairu-ms · 2022-06-24T01:49:04Z

src/amg/azext_amg/_params.py


+    with self.argument_context("grafana notification-channel") as c:
+        c.argument("notification_channel", help="id, uid which can identify a data source. CLI will search in the order of id, and uid, till finds a match")
+        c.argument("definition", help="json string with notification channel definition, or a path to a file with such content")


You can use type=validate_file_or_dict for json string or json file.

azure-cli-extensions/src/datafactory/azext_datafactory/generated/_params.py

Lines 395 to 400 in 81dd55b

c.argument('activities', type=validate_file_or_dict, help='List of activities in pipeline. Expected value: '

'json-string/json-file/@json-file.')

c.argument('parameters', type=validate_file_or_dict, help='List of parameters for pipeline. Expected value: '

'json-string/json-file/@json-file.')

c.argument('variables', type=validate_file_or_dict, help='List of variables for pipeline. Expected value: '

'json-string/json-file/@json-file.')

Currently, none of the arguments have type validation in place. Would you like me to expand my scope slightly and add type validation to all pre-existing commands as well?

Yes, please. BTW, we usually don't need to specify type for most simple arguments.

Applied in commits [REV2]

Type validation of the argument was added, leading to improved error messaging but also to conflict with the _try_load_file_content function currently executing type validation. This function was therefore omitted for all cli arguments except 'az grafana dashboard import' as this command takes int, str and json objects.

kairu-ms · 2022-06-27T01:13:46Z

Could you add tests for your changes?

yugangw-msft · 2022-07-05T03:38:34Z

@michelletaal-shell your PR looks solid we would like to merge. Do you need any help on adding the test? Assume you have followed the dev doc, you can add a test in /tests/latest/test_amg_scenario.py, then run azdev test amg which should generate a new test recording yaml file under /tests/latest/recordings/. You can then add both files to the PR.

Or I can push the test change to your branch if you prefer.

michelletaal · 2022-07-05T08:26:36Z

@michelletaal-shell your PR looks solid we would like to merge. Do you need any help on adding the test? Assume you have followed the dev doc, you can add a test in /tests/latest/test_amg_scenario.py, then run azdev test amg which should generate a new test recording yaml file under /tests/latest/recordings/. You can then add both files to the PR.

Or I can push the test change to your branch if you prefer.

Thank you for accepting my pr.

I've been working on writing some tests, but have been otherwise occupied the last few days. I've been having issues with the tests as automatically setting up the Managed Identity requires Azure permissions I don't have in my professional subscription. I'll be trying to work my way around that later this week. I suspect this is why there are also no tests for the other API-call functionalities. Once I get over that hump, the actual tests should be in place rather quickly.

yugangw-msft · 2022-07-08T05:20:41Z

@michelletaal-shell, sounds good. If the permission is hard to workaround, az grafana create has skip arguments to bypass those. Hope it helps.

yugangw-msft · 2022-07-14T06:40:25Z

@michelletaal-shell, let you know that we are on the way to move to Grafana 9.0 with unified alerting. This will light up the contact-point which is similar with notification channel. I suggest we hold on to this PR and I will work with you in a couple of weeks to migrate this PR to support contact-point. Will that work for you?

michelletaal · 2022-07-14T09:07:06Z

@yugangw-msft, thank you for the update. I'm nearly done with setting up tests for all features and will commit those this week. I'm definitely willing to work with you to support contact point. Just let me know when you guys are ready to move forward.

Pull from main

…/michelletaal-shell/azure-cli-extensions into feature/amg-notification-channel

kairu-ms · 2022-07-25T04:00:42Z

@yugangw-msft does this PR ready for review and merge?

yugangw-msft · 2022-07-25T16:41:30Z

@kairu-ms, please merge.
Note, I will have one more PR for AMG's GA feature to submit this week. Once that is in, I will bump the version to release these 2 change together.

michelletaal added 4 commits June 22, 2022 12:49

added notification feature to azure amg

9097bf8

change version setup

59fca2a

azdev style edits and update readme

77fa3f8

added help for new options and azdev lint edits

ee468a5

ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Jun 22, 2022

ghost requested a review from yonzhan June 22, 2022 14:37

ghost added the Auto-Assign Auto assign by bot label Jun 22, 2022

ghost requested a review from wangzelin007 June 22, 2022 14:37

ghost assigned kairu-ms Jun 22, 2022

ghost added this to the Jun 2022 (2022-07-05) milestone Jun 22, 2022

ghost added the extension/grafana az grafana label Jun 22, 2022

ghost requested review from jsntcy and kairu-ms June 22, 2022 14:37

fixed spelling plural to singular

c699f9b

yugangw-msft reviewed Jun 23, 2022

View reviewed changes

kairu-ms reviewed Jun 24, 2022

View reviewed changes

michelletaal added 3 commits June 24, 2022 18:12

[REV1] merge list command with short argument

23a2635

[REV2] Validate json string or file (+ int type)

3c03487

[REV2] Remove in-function json validation

40c8f99

Merge branch 'feature/amg-notification-channel' into main

842ab62

michelletaal added 6 commits July 18, 2022 15:17

Pull from main

dbdedca

Pull from main

added end-to-end testing

75f70f0

Merge branch 'feature/amg-notification-channel' of https://github.com…

335d486

…/michelletaal-shell/azure-cli-extensions into feature/amg-notification-channel

update e2e testing w/ pass

6cb91ce

update header; delete test output

d2fd7e6

set tests to live and base commands

f4e0d92

kairu-ms approved these changes Jul 26, 2022

View reviewed changes

kairu-ms merged commit b0ebdda into Azure:main Jul 26, 2022

	c.argument('activities', type=validate_file_or_dict, help='List of activities in pipeline. Expected value: '
	'json-string/json-file/@json-file.')
	c.argument('parameters', type=validate_file_or_dict, help='List of parameters for pipeline. Expected value: '
	'json-string/json-file/@json-file.')
	c.argument('variables', type=validate_file_or_dict, help='List of variables for pipeline. Expected value: '
	'json-string/json-file/@json-file.')

Conversation

michelletaal commented Jun 22, 2022

Related command

General Guidelines

About Extension Publish

Uh oh!

ghost commented Jun 22, 2022

Uh oh!

ghost commented Jun 22, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Jun 22, 2022

Uh oh!

yugangw-msft commented Jun 23, 2022

Uh oh!

yugangw-msft Jun 23, 2022

Choose a reason for hiding this comment

Uh oh!

michelletaal Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

kairu-ms Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

michelletaal Jun 24, 2022 • 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!

kairu-ms Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

michelletaal Jun 24, 2022

Choose a reason for hiding this comment

Uh oh!

kairu-ms commented Jun 27, 2022

Uh oh!

yugangw-msft commented Jul 5, 2022

Uh oh!

michelletaal commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yugangw-msft commented Jul 8, 2022

Uh oh!

yugangw-msft commented Jul 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michelletaal commented Jul 14, 2022

Uh oh!

kairu-ms commented Jul 25, 2022

Uh oh!

yugangw-msft commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Jun 22, 2022 •

edited by ghost

Loading

michelletaal Jun 24, 2022 •

edited

Loading

michelletaal commented Jul 5, 2022 •

edited

Loading

yugangw-msft commented Jul 14, 2022 •

edited

Loading