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

[ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) and add new sub commands for management-group #20942

Merged
merged 14 commits into from
Feb 22, 2022

Conversation

git-thomasdolan
Copy link
Contributor

@git-thomasdolan git-thomasdolan commented Jan 10, 2022

Description

  • Update the SDK version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0.
  • Add a new command subgroup az account management-group entities
  • Add a new command subgroup az account management-group hierarchy-settings
  • Add a new command subgroup az account management-group tenant-backfill
  • Add a new command az account management-group subscription show
  • Add a new command az account management-group subscription show-sub-under-mg
  • Add a new command az account management-group check-name-availability

Testing Guide

  • Check if "GroupName" is valid.
    cmd: az account management-group check-name-availability -n GroupName

  • Show the details of a subscription "subName" under a management group "GroupName"
    cmd: az account management-group subscription show -n GroupName -s subName

  • Get the subscription(s) under a management group "GroupName"
    cmd: az account management-group subscription show -n GroupName

  • Get the Backfill Status for the current tenant
    cmd: az account management-group tenant-backfill get

  • Start Backfilling Subscriptions for the current tenant
    cmd: az account management-group tenant-backfill start

  • List all entities for the authenticated user
    cmd: az account management-group entities list

  • Get all hierarchy settings under the "RootMG" tenant
    cmd: az account management-group hierarchy-settings list-n RootMG

  • Create a hierarchy setting to require write permissions on the "RootMG" tenant to create a new Management Group under it
    cmd: az account management-group hierarchy-settings create -n RootMG -r True

  • Create a hierarchy setting to set "defaultGroup" as the new MG groups get created under in the "RootMG" tenant
    cmd: az account management-group hierarchy-settings create -n RootMG -m /providers/Microsoft.Management/managementGroups/DefaultGroup

  • Update the hierarchy setting to require write permissions on the "RootMG" tenant to create a new Management Group under
    cmd: az account management-group hierarchy-settings update-n RootMG -r True

  • Update the hierarchy setting to set "defaultGroup" as the new MG groups get created under in the "RootMG" tenant
    cmd: az account management-group hierarchy-settings update -n RootMG -m /providers/Microsoft.Management/managementGroups/DefaultGroup

  • Delete the hierarchy settings for the "RootMG" tenant
    cmd: az account management-group hierarchy-settings delete -n RootMG

History Notes

[ARM] az account management-group entities: Add a new command group to support entities (Management Groups and Subscriptions) operations for the authenticated user
[ARM] az account management-group hierarchy-settings: Add a new command group to support operations on hierarchy settings defined at the management group level
[ARM] az account management-group tenant-backfill: Add a new command group to support backfilling subscriptions for the tenant
[ARM] az account management-group subscription show: Get the details of a given subscription under a given management group
[ARM] az account management-group subscription show-sub-under-mg: Show what subscription is under a given management group
[ARM] az account management-group check-name-availability: Check if a management group name is valid and available


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

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Jan 10, 2022
@ghost
Copy link

ghost commented Jan 10, 2022

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

@yonzhan
Copy link
Collaborator

yonzhan commented Jan 10, 2022

managementgroups

@yonzhan yonzhan added this to the Jan 2022 (2022-02-08) milestone Jan 10, 2022
@yonzhan yonzhan assigned zhoxing-ms and unassigned kairu-ms Jan 10, 2022
@zhoxing-ms zhoxing-ms changed the title Updated version number for azure-mgmt-managementgroups sdk {Compute} Bump version for azure-mgmt-managementgroups SDK Jan 11, 2022
@zhoxing-ms zhoxing-ms changed the title {Compute} Bump version for azure-mgmt-managementgroups SDK {Compute} Bump version for azure-mgmt-managementgroups (Track 2 SDK) Jan 11, 2022
@zhoxing-ms
Copy link
Contributor

ERROR: ************* Module azure.cli.command_modules.resource._exception_handler
src/azure-cli/azure/cli/command_modules/resource/_exception_handler.py:10:4: E0611: No name 'ErrorResponseException' in module 'azure.mgmt.managementgroups.models' (no-name-in-module)
src/azure-cli/azure/cli/command_modules/resource/_exception_handler.py:10:4: E0611: No name 'ErrorResponseException' in module 'azure.mgmt.managementgroups.models' (no-name-in-module)

@git-thomasdolan Hi, in the track 2 SDK, please replace from azure.mgmt.managementgroups.models import ErrorResponseException with from azure.core.exceptions import HttpResponseError to fix the CI issue

@zhoxing-ms zhoxing-ms changed the title {Compute} Bump version for azure-mgmt-managementgroups (Track 2 SDK) {Compute} Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) Jan 11, 2022
@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Jan 11, 2022

@git-thomasdolan In addition, since the version 1.0.0 of azure-mgmt-managementgroups is Track 2 SDK, there will be a large number of breaking changes in the SDK, you need to add corresponding compatible logic to account management-group xxx related commands.
For how to migrate Track 2 SDK, you can refer to #17783 (comment)

Then you need to re-run all the tests which include account management-group xxx command in module resource through live mode, so as to update the api-version in yaml file which no longer matches the actual value in SDK after upgrading the SDK version.

@git-thomasdolan
Copy link
Contributor Author

git-thomasdolan commented Jan 22, 2022

@zhoxing-ms I have the update method use the begin_create_or_update method in the SDK, after that it was able to pass the tests. If that is okay, then this is ready for review. The Azure Portal also seems to use the PUT method instead of PATCH when I checked so this would be consistent with Azure Portal

@git-thomasdolan git-thomasdolan marked this pull request as ready for review January 22, 2022 01:05
@zhoxing-ms zhoxing-ms changed the title {Compute} Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) {ARM} Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) Jan 25, 2022
@zhoxing-ms zhoxing-ms removed this from the Jan 2022 (2022-02-08) milestone Jan 27, 2022
@git-thomasdolan
Copy link
Contributor Author

Hi @zhoxing-ms, we fixed the parentId parameter issue in our API Controller code and now the PATCH MG route works. It is passing all the unit tests but now it is failing the credential scan. Do you know what causes this?

@zhoxing-ms
Copy link
Contributor

@git-thomasdolan This is a problem caused by credscan package Microsoft.Security.CredScan upgrade. We have submitted a PR #21230 to pin it

@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@git-thomasdolan
Copy link
Contributor Author

Can this be reviewed again now that all the tests have passed?

with self.command_group('account management-group hierarchy-settings', resource_hierarchy_settings_sdk, client_factory=cf_hierarchy_settings) as g:
g.custom_command('list', 'cli_hierarchy_settings_list')
g.custom_command('create', 'cli_hierarchy_settings_create')
g.custom_command('delete', 'cli_hierarchy_settings_delete')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to consider add confirmation=True for the delete command?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it

@zhoxing-ms zhoxing-ms changed the title {ARM} Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) [ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) Feb 11, 2022
Copy link
Contributor

@zhoxing-ms zhoxing-ms left a comment

Choose a reason for hiding this comment

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

@git-thomasdolan I see that there are some new commands in this PR. Could you please refer to the specification of this doc format-pr-description write what needs to be written into the history notes into the PR description

@git-thomasdolan
Copy link
Contributor Author

Is my updated title/description okay? I wasn't sure if it was correct from the guideline you linked. I referred to some previous PRs for some help

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Feb 16, 2022

[ARM] Update the SDK version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0.

@git-thomasdolan Bumping SDK version doesn't need to be written into history notes, so please remove this item

[ARM] Add a new command subgroup az account management-group entities
[ARM] Add a new command subgroup az account management-group hierarchy-settings
[ARM] Add a new command subgroup az account management-group tenant-backfill
[ARM] Add a new command az account management-group subscription show
[ARM] Add a new command az account management-group subscription show-sub-under-mg
[ARM] Add a new command az account management-group check-name-availability

Please describe the specific features of the new commands and command groups
Such as:

[ARM] account management-group tenant-backfill: Add new command groups to support backfill tenant subscription operations

Comment on lines -169 to -173
managementgroup_create["properties"]["tenantId"])
managementgroup_create["tenantId"])
self.assertEqual(
managementgroup_create["properties"]["details"]["parent"]["name"],
managementgroup_create["properties"]["tenantId"])
self.assertIsNotNone(managementgroup_create["properties"]["tenantId"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete ["properties"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The json formatting changed when updating the API version, removing [properties]

@zhoxing-ms
Copy link
Contributor

@git-thomasdolan Could you please address those conflicts?

@zhoxing-ms zhoxing-ms changed the title [ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) [ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) and add new sub commands for management-group Feb 18, 2022
@git-thomasdolan git-thomasdolan changed the title [ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) and add new sub commands for management-group [ARM] Bump version of azure-mgmt-managementgroups from 0.2.0 to 1.0.0 (Track 2 SDK) and add new sub commands for management-group Feb 18, 2022
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 20942 in repo Azure/azure-cli

@git-thomasdolan
Copy link
Contributor Author

@zhoxing-ms Can you re-run the pipeline? The new title started with a space so it caused the tests to fail. I don't have permissions to re-run

@zhoxing-ms zhoxing-ms merged commit 03932ec into Azure:dev Feb 22, 2022
helps['account management-group tenant-backfill get'] = """
type: command
short-summary: Get the backfill status for a tenant.
long-summary: Get the backfill status for a tenant.
Copy link
Member

Choose a reason for hiding this comment

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

long-summary should provide additional information, instead of being a duplicate of short-summary. See #6426 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants