[Resources/locks] Added GetAtResourceLevel #666

mcardosos · 2016-11-03T21:54:30Z

This PR would fix issue 394. In the Go SDK, this would also fix issue 440 when regenerating the SDK.

This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.

PR information

The title of the PR is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
If applicable, the PR references the bug/issue that it fixes.
Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- Validation errors from the Linter extension for VS Code have all been fixed for this spec. (Note: for large, previously checked in specs, there will likely be many errors shown. Please contact our team so we can set a timeframe for fixing these errors if your PR is not going to address them).
- The spec follows the patterns described in the Swagger good patterns document unless the service API makes this impossible.

amarzavery · 2016-11-03T23:55:35Z

arm-resources/locks/2016-09-01/swagger/locks.json

@@ -325,6 +325,76 @@
            "description": "OK"
          }
        }
+      },
+      "get": {


@vivsriaus @TianoMS - Looks like these APIs were missing. @mcardosos from the Azure Go SDK team found this by looking at the MSDN REST API docs. Can you please review the changes?

"/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{parentResourcePath}/{resourceType}/{resourceName}/providers/Microsoft.Authorization/locks/{lockName}"

"{resourceId}/providers/Microsoft.Authorization/locks/{lockName}"

@TianoMS @vivsriaus - Do you think having the version with resourceId would be helpful ?

@amarzavery Yes, we've received feedback from people, saying the methods that take in parentResourcePath is not at all intuitive. It is pretty cumbersome to construct the request URI for the above using parentResourcePath. A simpler implementation would be the one suggested by you (similar to this) or a path that points to a a fully qualified /{lockId}

amarzavery · 2016-11-04T00:04:12Z

arm-resources/locks/2016-09-01/swagger/locks.json

+            "in": "path",
+            "required": true,
+            "type": "string",
+            "description": "Resource identity.",


copy paste error in description.This needs to be fixed for the entire spec.

amarzavery · 2016-11-07T21:20:32Z

arm-resources/locks/2016-09-01/swagger/locks.json

@@ -407,8 +596,8 @@
        "tags": [
          "ManagementLocks"
        ],
-        "operationId": "ManagementLocks_Get",
-        "description": "Gets the management lock of a scope.",
+        "operationId": "ManagementLocks_GetAtSubscriptionLevel",


@vivsriaus @TianoMS -
This will be a breaking change in the SDK. It is the correct change though. Is this acceptable?

amarzavery · 2016-11-24T01:00:38Z

@vivsriaus @TianoMS - Can you please review these fixes and let us know the status?

vivsriaus · 2016-11-24T01:21:49Z

arm-resources/locks/2016-09-01/swagger/locks.json

@@ -173,6 +173,125 @@
        }
      }
    },
+    "/{resourceId}/providers/Microsoft.Authorization/locks/{lockName}": {


can we please replace {resourceId} with {scope}. In ARM specs, resourceId signifies the fully qualified Id of a resource

vivsriaus · 2016-11-24T01:22:39Z

@amarzavery Just one nit, other than that this LGTM

mcardosos · 2016-11-24T01:44:13Z

@vivsriaus done

amarzavery · 2016-11-27T06:34:05Z

@lmazuel, @johanste, @veronicagg, @mcardosos @markcowl @vivsriaus @jianghaolu

FYI,

This PR will cause a breaking change in the generated SDK. However, this is a good breaking change. If all of you guys agree to this then feel free to merge the PR at the earliest.

The change is as follows:
"ManagementLocks_Get" -> will be renamed to "ManagementLocks_GetAtSubscriptionLevel" and a new method is being added "ManagementLocks_GetAtResourceLevel".

It will be a breaking change for the SDKs and an easy to fix change inside the Powershell and Xplat CLI/ Py CLI command implementation (Possibly, No breaking change to the command API).

In fact, new optional parameters/switches can be added to list management locks at the resource level.

veronicagg

Just left a comment, rest looks good to me. It does have a breaking change for SDKs as you pointed out, so we should make sure all SDKs are ok with this.

A couple more questions:
1 - I assume the call to the API with resource ID is working, correct? @mcardosos
2 - With "scope" does it mean we could remove the other paths that are more specific? Calling it 'scope' looks to follow a similar format as in'policy management', which appears to group all scopes into one path, and describe what scope could be in the documentation (per https://docs.microsoft.com/en-us/rest/api/resources/policyassignments).

veronicagg · 2016-12-07T23:34:19Z

arm-resources/locks/2016-09-01/swagger/locks.json

+            "in": "path",
+            "required": true,
+            "type": "string",
+            "description": "The scope for the lock. "


should the description for scope indicate what "scope" string should look like?

good point an example helps.

The scope for the lock, where the scope is the unique Azure Resource id in the form "/subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{parentResourcePath}/{resourceType}/{resourceName}".

@mcardosos - ROFL . I made the same comment as you have fixed. Your fix and my comment were 4 seconds apart.

veronicagg · 2016-12-08T22:04:53Z

arm-resources/locks/2016-09-01/swagger/locks.json

@@ -186,7 +186,7 @@
            "in": "path",
            "required": true,
            "type": "string",
-            "description": "The scope for the lock. "
+            "description": "The scope for the lock. A scope can be defined at subscription, resource group, or resource level. It looks like this: /subscriptions/{subscriptionId}/resourcegroups/{resourceGroupName}/providers/{resourceProviderNamespace}/{parentResourcePathIfPresent}/{resourceType}/{resourceName}"


Thanks! Are these other scope strings also valid? Extract below taken from policy management documentation:
"When providing a scope for the assignment, use '/subscriptions/{subscription-id}/' for subscriptions, '/subscriptions/{subscription-id}/resourceGroups/{resource-group-name}' for resource groups, and '/subscriptions/{subscription-id}/resourceGroups/{resource-group-name}/providers/{resource-provider-namespace}/{resource-type}/{resource-name}' for resources."

All those are valid yes. Would you prefer that more complete description?

@mcardosos Yes, it'd be good to have a more complete description, as it may get picked up for documentation, and this way users have more details.

salameer · 2016-12-09T09:33:24Z

Hi @veronicagg are we ready to merge here?

veronicagg · 2016-12-09T18:39:44Z

@salameer I made one more comment for @mcardosos. This change is a breaking change though, and as @amarzavery mentioned above it may require some changes to PowerShell/CLI, so we should know that all SDKs approve making this change, before merging.

AutorestCI · 2016-12-21T00:50:53Z

No modification for Python

AutorestCI · 2016-12-21T00:51:32Z

No modification for NodeJS

AutorestCI · 2016-12-21T00:52:28Z

Azure/azure-sdk-for-ruby#634

[Resources/locks] Added GetAtResourceLevel

0b1c6d4

azurecla added the cla-not-required label Nov 3, 2016

amarzavery reviewed Nov 3, 2016

View reviewed changes

amarzavery reviewed Nov 4, 2016

View reviewed changes

[Resources/locks] Fixed descriptions and added ById operations

0d01c1a

amarzavery reviewed Nov 7, 2016

View reviewed changes

Merge master

de81df8

vivsriaus reviewed Nov 24, 2016

View reviewed changes

[Resources/locks] replaced resourceID with scope

3b13a80

amarzavery added the potential-sdk-breaking-change label Nov 27, 2016

veronicagg approved these changes Dec 7, 2016

View reviewed changes

[Resources/Locks] More complete scope description

f5ba0af

veronicagg reviewed Dec 8, 2016

View reviewed changes

[Resources/Locks] Even more complete scope description

7a8737b

amarzavery merged commit d7903f7 into Azure:master Dec 21, 2016

AutorestCI mentioned this pull request Dec 21, 2016

Automatic PR from RestAPI-PR666 Azure/azure-sdk-for-ruby#634

Merged

mcardosos deleted the locks branch September 12, 2017 23:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Resources/locks] Added GetAtResourceLevel #666

[Resources/locks] Added GetAtResourceLevel #666

mcardosos commented Nov 3, 2016 •

edited by fearthecowboy

Loading

amarzavery Nov 3, 2016

amarzavery Nov 4, 2016

vivsriaus Nov 4, 2016

amarzavery Nov 4, 2016

amarzavery Nov 7, 2016

amarzavery commented Nov 24, 2016

vivsriaus Nov 24, 2016

vivsriaus commented Nov 24, 2016

mcardosos commented Nov 24, 2016

amarzavery commented Nov 27, 2016 •

edited

Loading

veronicagg left a comment •

edited

Loading

veronicagg Dec 7, 2016

amarzavery Dec 7, 2016

mcardosos Dec 7, 2016

amarzavery Dec 7, 2016

veronicagg Dec 8, 2016

mcardosos Dec 8, 2016

veronicagg Dec 9, 2016

mcardosos Dec 9, 2016

salameer commented Dec 9, 2016

veronicagg commented Dec 9, 2016

AutorestCI commented Dec 21, 2016

AutorestCI commented Dec 21, 2016

AutorestCI commented Dec 21, 2016

[Resources/locks] Added GetAtResourceLevel #666

[Resources/locks] Added GetAtResourceLevel #666

Conversation

mcardosos commented Nov 3, 2016 • edited by fearthecowboy Loading

PR information

Quality of Swagger

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarzavery commented Nov 24, 2016

Choose a reason for hiding this comment

vivsriaus commented Nov 24, 2016

mcardosos commented Nov 24, 2016

amarzavery commented Nov 27, 2016 • edited Loading

veronicagg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salameer commented Dec 9, 2016

veronicagg commented Dec 9, 2016

AutorestCI commented Dec 21, 2016

AutorestCI commented Dec 21, 2016

AutorestCI commented Dec 21, 2016

mcardosos commented Nov 3, 2016 •

edited by fearthecowboy

Loading

amarzavery commented Nov 27, 2016 •

edited

Loading

veronicagg left a comment •

edited

Loading