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

[Automation] Refactoring Automation Resources #3190

Merged
merged 15 commits into from
Jun 22, 2018
Merged

[Automation] Refactoring Automation Resources #3190

merged 15 commits into from
Jun 22, 2018

Conversation

vrdmr
Copy link
Member

@vrdmr vrdmr commented Jun 7, 2018

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

@AutorestCI
Copy link

AutorestCI commented Jun 7, 2018

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

@AutorestCI
Copy link

AutorestCI commented Jun 7, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Jun 7, 2018

Automation for azure-sdk-for-node

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

@AutorestCI
Copy link

AutorestCI commented Jun 7, 2018

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
Copy link

AutorestCI commented Jun 7, 2018

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.

@vrdmr vrdmr changed the title [Automation] Refactoring Automation Resources [Do-Not-Merge][Automation] Refactoring Automation Resources Jun 7, 2018
Copy link
Contributor

@jianghaolu jianghaolu left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

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

Choose a reason for hiding this comment

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

There's no automationAccountName needed here.

@vrdmr vrdmr changed the title [Do-Not-Merge][Automation] Refactoring Automation Resources [Automation] Refactoring Automation Resources Jun 9, 2018
},
"allOf": [
{
"$ref": "../../common/v1/definitions.json#/definitions/ProxyResource"
Copy link
Member Author

Choose a reason for hiding this comment

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

@DeveloperTommy Added Schedule as a ProxyResource.

}
],
"responses": {
"201": {
"200": {
Copy link
Member Author

@vrdmr vrdmr Jun 9, 2018

Choose a reason for hiding this comment

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

@DeveloperTommy Changed the 201 to 200.

@@ -18,7 +18,7 @@
}
},
"responses": {
"201": {
"200": {
Copy link
Member Author

Choose a reason for hiding this comment

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

@DeveloperTommy Changed 201 to 200.

@vrdmr
Copy link
Member Author

vrdmr commented Jun 11, 2018

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

}
},
"201": {
"description": "Software update configuration is created.",
"schema": {
"$ref": "./definitions.json#/definitions/softwareUpdateConfiguration"
"$ref": "#/definitions/softwareUpdateConfiguration"
}
},
"default": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -214,7 +214,7 @@
"default": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

},
"Key": {
"properties": {
"keyName": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

"required": true,
"type": "string",
"description": "The automation account name."
"$ref": "../../common/v1/definitions.json#/parameters/AutomationAccountNameParameter"
},
{
"name": "credentialName",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -350,7 +334,7 @@
"description": "Name of the node configuration."
},
"configuration": {
"$ref": "./definitions.json#/definitions/DscConfigurationAssociationProperty",
"$ref": "#/definitions/DscConfigurationAssociationProperty",
"description": "Gets or sets the configuration of the node."
},
"incrementNodeConfigurationBuild": {
Copy link
Contributor

Choose a reason for hiding this comment

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

"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

"x-ms-enum": {
"name": "JobProvisioningState",
"modelAsString": true
}
},
"JobCreateParameters": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll verify and confirm tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Confirmed and removed.

@@ -53,14 +53,10 @@
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

}
],
"responses": {
"200": {
"description": "A single software update configuration.",
"schema": {
"$ref": "./definitions.json#/definitions/softwareUpdateConfiguration"
"$ref": "#/definitions/softwareUpdateConfiguration"
}
},
"404": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@@ -266,7 +266,7 @@
"200": {
"description": "Return list of software update configurations.",
"schema": {
"$ref": "./definitions.json#/definitions/softwareUpdateConfigurationListResult"
"$ref": "#/definitions/softwareUpdateConfigurationListResult"
}
},
"404": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

}
],
"responses": {
"200": {
"200": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}
],
"responses": {
"200": {
"200": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@@ -443,7 +443,7 @@
"$ref": "#/definitions/DscConfigurationAssociationProperty",
"description": "Gets or sets the configuration of the node."
},
"incrementNodeConfigurationBuild": {
"IncrementNodeConfigurationBuild": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@vrdmr
Copy link
Member Author

vrdmr commented Jun 15, 2018

@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
Copy link
Contributor

@jianghaolu
Copy link
Contributor

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

@jianghaolu
Copy link
Contributor

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
Copy link

AutorestCI commented Jun 18, 2018

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@vrdmr
Copy link
Member Author

vrdmr commented Jun 19, 2018

@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.

@@ -247,9 +247,6 @@
"200": {
"description": "OK"
},
"404": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! Signing off.

where: $.definitions.TestJob.properties
- suppress: DefinitionsPropertiesNamesCamelCase

Choose a reason for hiding this comment

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

signing off on this change

Copy link

@balukambala balukambala left a comment

Choose a reason for hiding this comment

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

Signing off on suppression changes on camel case

Copy link
Contributor

@finiteattractor finiteattractor left a comment

Choose a reason for hiding this comment

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

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

@jianghaolu jianghaolu merged commit ac8857e into Azure:master Jun 22, 2018
@Francisco-Gamino
Copy link
Contributor

Thanks @jianghaolu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants