[Automation] Refactoring Automation Resources by vrdmr · Pull Request #3190 · Azure/azure-rest-api-specs

vrdmr · 2018-06-07T06:34:51Z

Refactoring Automation Resources to prevent the duplication of Models in different definitions file and creating a single common definition file for definitions/parameters used across Automation.

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.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-06-07T06:35:30Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

AutorestCI · 2018-06-07T06:35:49Z

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#1959

AutorestCI · 2018-06-07T06:36:28Z

Automation for azure-sdk-for-node

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-node#2653

AutorestCI · 2018-06-07T06:37:11Z

Automation for azure-sdk-for-go

Encountered a Subprocess error: (azure-sdk-for-go)

Command: ['/usr/local/bin/autorest', '/tmp/tmp24yyqvao/rest/specification/automation/resource-manager/readme.md', '--go', '--go-sdk-folder=/tmp/tmp24yyqvao/src/github.com/Azure/azure-sdk-for-go', '--multiapi', '--use=@microsoft.azure/autorest.go@~2.1.100', '--use-onever', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
   Loading AutoRest extension '@microsoft.azure/autorest.go' (~2.1.100->2.1.104)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
Processing batch task - {"tag":"package-2015-10"} .

ERROR (Fatal/DuplicateModelCollsion): Duplicated model name with non-identical definitions
    - file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/runbook.json:1056:4 ($.definitions.JobStreamProperties)
    - file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/job.json:780:4 ($.definitions.JobStreamProperties)
    - file:///tmp/tmp24yyqvao/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/dscCompilationJob.json:561:4 ($.definitions.JobStreamProperties)
Process() cancelled due to exception : Cancellation requested.
Failure during batch task - {"tag":"package-2015-10"} -- Cancellation requested..
  Cancellation requested.

AutorestCI · 2018-06-07T06:37:12Z

Automation for azure-sdk-for-ruby

Encountered a Subprocess error: (azure-sdk-for-ruby)

Command: ['/usr/local/bin/autorest', '/tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/readme.md', '--multiapi', '--ruby', '--ruby-sdks-folder=/tmp/tmp86k9cmzb/sdk', '--use=@microsoft.azure/autorest.ruby@3.0.20', '--version=preview']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4280; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4280/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4280)
   Loading AutoRest extension '@microsoft.azure/autorest.ruby' (3.0.20->3.0.20)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.1.22->2.1.22)
Processing batch task - {"tag":"package-2015-10"} .

ERROR (Fatal/DuplicateModelCollsion): Duplicated model name with non-identical definitions
    - file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/runbook.json:1056:4 ($.definitions.JobStreamProperties)
    - file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/job.json:780:4 ($.definitions.JobStreamProperties)
    - file:///tmp/tmp86k9cmzb/rest/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/dscCompilationJob.json:561:4 ($.definitions.JobStreamProperties)
Process() cancelled due to exception : Cancellation requested.
Failure during batch task - {"tag":"package-2015-10"} -- Cancellation requested..
  Cancellation requested.

jianghaolu

Please fix the semantic errors since they are preventing further validations to be run.

jianghaolu · 2018-06-08T20:38:17Z

-      },
-      "description": "The response model for the list job operation."
-    },
-    "DscConfigurationAssociationProperty": {


Remove this property invalidates https://github.com/Azure/azure-rest-api-specs/pull/3190/files#diff-09777627978972db514896bb1119bc44R269

jianghaolu · 2018-06-08T20:45:11Z

+            "$ref": "../../common/v1/definitions.json#/parameters/ResourceGroupNameParameter"
+          },
+          {
+            "$ref": "../../common/v1/definitions.json#/parameters/AutomationAccountNameParameter"


There's no automationAccountName needed here.

vrdmr · 2018-06-09T22:35:46Z

+      },
+      "allOf": [
+        {
+          "$ref": "../../common/v1/definitions.json#/definitions/ProxyResource"


@DeveloperTommy Added Schedule as a ProxyResource.

vrdmr · 2018-06-09T22:36:16Z

        ],
        "responses": {
-          "201": {
+          "200": {


@DeveloperTommy Changed the 201 to 200.

vrdmr · 2018-06-09T22:37:04Z

  },
  "responses": {
-    "201": {
+    "200": {


@DeveloperTommy Changed 201 to 200.

vrdmr · 2018-06-11T19:08:32Z

@jianghaolu Could you please take a look at this PR. Thanks.

jianghaolu · 2018-06-12T00:39:40Z

+              "$ref": "#/definitions/softwareUpdateConfiguration"
            }
          },
          "default": {


400, 404, and 409 are not defined here so please remove them from https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/examples/softwareUpdateConfiguration/createSoftwareUpdateConfiguration.json

jianghaolu · 2018-06-12T00:42:44Z

@@ -214,7 +214,7 @@
          "default": {


404 is not defined here but exists in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/preview/2017-05-15-preview/examples/softwareUpdateConfiguration/deleteSoftwareUpdateConfiguration.json

jianghaolu · 2018-06-12T00:44:36Z

+    },
+    "Key": {
+      "properties": {
+        "keyName": {


These are lower case properties but they are upper case in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/listAutomationAccountKeys.json

Hi @jianghaolu - Our service returns the properties in Uppercase, and if I define the property in upper case, I get the following error -
ERROR (DefinitionsPropertiesNamesCamelCase/R3016/RPCViolation): Property named: 'KeyName', for definition: 'Key' must follow camelCase style. Example: 'keyName'.

Is it a blocking issue or something which can leave it as it is in this API version and we can fix in the newer API version? Please let me know.

That error is simply a style error - but if your swagger is not corresponding to the actual service then developers won't be able to use the clients generated from this.

You might want to follow the suppression process to suppress this R3016 error: https://github.com/Azure/adx-documentation-pr/wiki/Swagger-Validation-Errors-Suppression

jianghaolu · 2018-06-12T00:46:07Z

+            "$ref": "../../common/v1/definitions.json#/parameters/AutomationAccountNameParameter"
          },
          {
            "name": "credentialName",


This parameter is missing in the example https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/deleteCredentialExisting.json

jianghaolu · 2018-06-12T00:53:53Z

+          "$ref": "#/definitions/DscConfigurationAssociationProperty",
          "description": "Gets or sets the configuration of the node."
        },
        "incrementNodeConfigurationBuild": {


"DscNodeConfigurationCreateOrUpdateParameters" doesn't contain a property called "newNodeConfigurationBuildVersionRequired" but it's present in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/createOrUpdateDscNodeConfiguration.json

jianghaolu · 2018-06-12T01:01:34Z

+        "modelAsString": true	
+      }
    },
    "JobCreateParameters": {


Missing properties "location", "name", "tags": https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/createJob.json#L18

@jianghaolu Its a proxy resource and would not have "Location", "Name" and "Tags".

But in the example I linked above there is - you might want to double check if it's a proxy resource.

I'll verify and confirm tomorrow.

Confirmed and removed.

jianghaolu · 2018-06-12T01:10:59Z

@@ -53,14 +53,10 @@
        },


Wrong example file reference? This operation returns a file for 200.

We need to discuss this issue. Can we sync offline tomorrow?

jianghaolu · 2018-06-12T01:18:25Z

+            "$ref": "../../common/v1/definitions.json#/parameters/ApiVersionParameter"
          }
        ],
        "responses": {


"JobListResult" contains Job, which doesn't have properties "schedule", "jobScheduleId" listed in the example: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples/listJobsByAutomationAccount.json

I've fixed this. But we have a new version of the Jobs resource 2017-05-15-preview and we have updated the SDK to use the new version as well.

jianghaolu · 2018-06-12T01:22:18Z

+          "description": "Gets or sets a Boolean value that indicates true if the parameter is dynamic."
+        },
+        "position": {
+          "type": "boolean",


This is defined as boolean but in the example they are all integers: getActivityInAModulehttps://github.com/vrdmr/azure-rest-api-specs/tree/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2015-10-31/examples

jianghaolu · 2018-06-13T21:04:42Z

+              "$ref": "#/definitions/softwareUpdateConfiguration"
            }
          },
          "404": {


status codes "404" for operation "SoftwareUpdateConfigurations_GetByName" were present in the swagger spec, however they were not present in x-ms-examples.

jianghaolu · 2018-06-13T21:05:11Z

+              "$ref": "#/definitions/softwareUpdateConfigurationListResult"
            }
          },
          "404": {


jianghaolu · 2018-06-13T21:24:16Z

        ],
        "responses": {
-          "200": {            
+          "200": {


Please also add a 200 example response in examples/replaceRunbookDraftContent.json

It never generates 200. This was added to fix the linting issues in #2687.

If your service doesn't generate 200 you shouldn't put it in the spec. If the linter reports error then we should fix the linter.

Fixed with a suppression at for LongRunningResponseStatusCode in readme.md for this.

jianghaolu · 2018-06-13T21:24:55Z

        ],
        "responses": {
-          "200": {            
+          "200": {


Same as above: It never generates 200. This was added to fix the linting issues in #2687.

jianghaolu · 2018-06-13T21:28:23Z

          "description": "Gets or sets the configuration of the node."
        },
-        "incrementNodeConfigurationBuild": {
+        "IncrementNodeConfigurationBuild": {


Please update the corresponding fields in this example too: https://github.com/vrdmr/azure-rest-api-specs/blob/0489a8a2c0bf758e9593a7d21ff507e0899ff698/specification/automation/resource-manager/Microsoft.Automation/stable/2018-01-15/examples/createOrUpdateDscNodeConfiguration.json

vrdmr · 2018-06-15T03:26:39Z

@jianghaolu Could you please take a look at this PR. We have to get the fixes in by Friday. Please let me know if you have any questions.

jianghaolu · 2018-06-15T23:03:13Z

@vrdmr Sorry for the delay - Did you get a chance to confirm this? https://github.com/Azure/azure-rest-api-specs/pull/3190/files/0489a8a2c0bf758e9593a7d21ff507e0899ff698#diff-91d25383f8afa4ecf50a11d676f50454

jianghaolu · 2018-06-15T23:03:56Z

There are a few other errors reported in the validator which I'm trying to understand.

jianghaolu · 2018-06-15T23:10:18Z

It seems like one error that the model validator is reporting is related to all the "advancedSchedule": null in the examples. I tested it locally and it seems like removing those will resolve the errors from the model validator.

AutorestCI · 2018-06-18T22:47:02Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

vrdmr · 2018-06-19T01:25:57Z

@jianghaolu The remaining errors in the Travis model check are looking at Job.json from the older 2015-10-31 folder and we don't even refer in the readme.md for the same. There is a new version in 2017-05-15-preview which we refer. I do not understand why its referring to that model, as they will not be changed.

DeveloperTommy · 2018-06-20T18:15:15Z

          "200": {
            "description": "OK"
          },
-          "404": {


I'm confused about why we are removing 404 here. If we try to get a schedule, shouldn't we return a 404 if it doesn't exist?

In the specs, we only specify success cases and not the failure cases. If we start specifying error cases, the SDK does not throw an exception and treats it as a normal case.

For more info, @finiteattractor can describe it in more details.

Thank you! Signing off.

balukambala · 2018-06-20T18:36:00Z

-    from: definitions.json
+    from: runbook.json
    where: $.definitions.TestJob.properties
+  - suppress: DefinitionsPropertiesNamesCamelCase


signing off on this change

balukambala

Signing off on suppression changes on camel case

finiteattractor

Thanks Varad for doing this big refactoring! Changes look good to me.

Francisco-Gamino · 2018-06-22T21:55:51Z

Thanks @jianghaolu!

Adding new common definitions file

7eb76dd

bsiegel added the review label Jun 7, 2018

azuresdkci assigned jianghaolu Jun 7, 2018

azuresdkci requested a review from jianghaolu June 7, 2018 06:35

Refactoring Automation Resources

cf70daa

This was referenced Jun 7, 2018

[AutoPR automation/resource-manager] [Automation] Refactoring Automation Resources Azure/azure-sdk-for-ruby#1352

Closed

[AutoPR automation/resource-manager] [Automation] Refactoring Automation Resources Azure/azure-sdk-for-node#2954

Merged

vrdmr changed the title ~~[Automation] Refactoring Automation Resources~~ [Do-Not-Merge][Automation] Refactoring Automation Resources Jun 7, 2018

vrdmr added 2 commits June 7, 2018 15:35

Fix 2017 resources and remove definitions.json

358420b

2018 Resources refactored

30f2739

jianghaolu suggested changes Jun 8, 2018

View reviewed changes

vrdmr and others added 2 commits June 8, 2018 22:00

Refactoring 2015-10-31 Resources

3f4969b

Correcting some more 2015 resources

5e0944c

vrdmr changed the title ~~[Do-Not-Merge][Automation] Refactoring Automation Resources~~ [Automation] Refactoring Automation Resources Jun 9, 2018

Corrected the runbook.json and defintions

8988721

AutorestCI mentioned this pull request Jun 9, 2018

[AutoPR automation/resource-manager] [Automation] Refactoring Automation Resources Azure/azure-sdk-for-python#2712

Merged

Fixing the schedule issues

0489a8a

vrdmr commented Jun 9, 2018

View reviewed changes

jianghaolu suggested changes Jun 12, 2018

View reviewed changes

Fixing based on review comments

9bf9ed3

vrdmr mentioned this pull request Jun 13, 2018

[Automation] Provisioning State Enum to String in DSCCompilationJob #3111

Closed

10 tasks

Correct the DscNodeConfiguration example

42a5fac

jianghaolu reviewed Jun 13, 2018

View reviewed changes

Fixed based on review comments.

ae1e983

vrdmr added 2 commits June 18, 2018 14:51

Fix the createJob.json

2ea81c8

Removing the advancedSchedule. No need in the example

57dea53

vrdmr added 2 commits June 18, 2018 16:51

Fixing SUC examples and Schedule definition errors

869cdb8

Adding a suppression for the LongRunningResponseStatusCode

5bf6dcb

DeveloperTommy reviewed Jun 20, 2018

View reviewed changes

balukambala reviewed Jun 20, 2018

View reviewed changes

balukambala approved these changes Jun 20, 2018

View reviewed changes

DeveloperTommy approved these changes Jun 20, 2018

View reviewed changes

finiteattractor approved these changes Jun 20, 2018

View reviewed changes

jianghaolu added the potential-sdk-breaking-change label Jun 22, 2018

jianghaolu approved these changes Jun 22, 2018

View reviewed changes

jianghaolu merged commit ac8857e into Azure:master Jun 22, 2018

This was referenced Jun 26, 2018

New version release for Automation SDK - 3.0.4-preview Azure/azure-sdk-for-net#4496

Merged

Reverting the schedule.json response code for Created to 201 as that … #3312

Merged

New Release of Automation SDK Azure/azure-sdk-for-net#4521

Closed

Conversation

vrdmr commented Jun 7, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-libraries-for-java

Uh oh!

AutorestCI commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

AutorestCI commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-ruby

Uh oh!

jianghaolu 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!

vrdmr Jun 9, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

vrdmr commented Jun 11, 2018

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!

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

vrdmr commented Jun 7, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

AutorestCI commented Jun 7, 2018 •

edited

Loading

vrdmr Jun 9, 2018 •

edited

Loading