Fix Automation swagger spec errors in the AutomationAccount and Webhook areas #2360

AnatoliB · 2018-01-30T00:53:20Z

Fixing the following errors in the Automation swagger spec:

(PatchBodyParametersSchema/R2016/RPCViolation): Properties of a PATCH request body must not be required. PATCH operation: 'AutomationAccount_Update' Model Definition: 'AutomationAccountUpdateParameters' Property: 'properties'
(PatchBodyParametersSchema/R2016/RPCViolation): Properties of a PATCH request body must not be required. PATCH operation: 'Webhook_Update' Model Definition: 'WebhookUpdateParameters' Property: 'name'
(XmsResourceInPutResponse/R2062/RPCViolation): The 200 response model for an ARM PUT operation must have x-ms-azure-resource extension set to true in its hierarchy. Operation: 'Webhook_CreateOrUpdate' Model: 'Webhook'

The intent of this PR is to address these errors only. The entire Automation swagger spec contains other errors, and they will be addressed by separate PRs.

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

azuresdkciprbot · 2018-01-30T00:59:30Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

File: specification/automation/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 54
After the PR: Warning(s): 0 Error(s): 51

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

AutorestCI · 2018-01-30T01:22:18Z

Did a commit to Azure/azure-sdk-for-go:
Azure/azure-sdk-for-go@c4a7f98

AutorestCI · 2018-01-30T01:25:49Z

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@d4c20d6

sergey-shandar · 2018-01-30T19:06:11Z

@AnatoliB are you aware that this may introduce SDK breaking changes for such languages like C#, Java, Go?

AnatoliB · 2018-01-30T19:50:49Z

@sergey-shandar Currently we (Automation team) are in the process of releasing the Autorest generated client to the public and do not have any external users. Also, our Autorest-SDKs are in preview mode currently to help us fix such issues. Thus, this change should not be a problem.

sergey-shandar · 2018-01-31T02:01:59Z

@AnatoliB Thank you, it's good to know that this is preview. I assume, the file path should be .../preview/2015-10-31/definitions.json instead of .../stable/2015-10-31/definitions.json.

@marstr Do you know why this is in the stable folder? Should we move it to preview?

marstr · 2018-01-31T17:28:43Z

@sergey-shandar says:
@marstr Do you know why this is in the stable folder? Should we move it to preview?

If there are no external customers, moving it into preview is the appropriate course of action.

edit: formatting

vrdmr · 2018-01-31T20:30:03Z

Hi @marstr /@sergey-shandar

We are in the process of generating a new SDK using Swagger spec (Azure/azure-sdk-for-net#3992), which would be in the preview, where it (SDK) will be promoted out of preview when our PowerShell cmdlets and internal testing tools would be migrated.

But our API for 2015-10-31 is stable (https://github.com/Azure/azure-resource-manager-schemas/tree/master/schemas/2015-10-31) and are public. The changes that @AnatoliB is doing is to fix the linting issues with the spec such that it complies with the service. If there are any service changes, which require a major API change, we would be adding that change in the preview/2017-05-15-preview version itself.

Please let me know if you have any questions or suggestions for fixing the linter issues in the existing spec.

sergey-shandar

@vrdmr thank you for the context. In this case, I would suggest not to introduce the changes in the old API version if they can bring SDK breaking changes.

Making a property optional (removing from required) is a breaking change for some programming languages, for example C#.
I'm not sure about "x-ms-azure-resource": true. It may introduce breaking changes for some programming languages.

Definetly this changes should be in the new API version but it doesn't make much sense to introduce SDK breaking changes for your customers.

Let me know, if there are other reasons (except fixing linter errors/warning) to introduce the changes in the stable API.

AnatoliB · 2018-02-01T19:27:28Z

My understanding from conversations with @vrdmr is that:

The web service API is stable and public, and this spec does not introduce any change.
The SDK based on this swagger spec has never been officially released yet.

So, any existing web service client will continue working against (1) as it is still communicating with exactly the same service, and it is not even aware of (2). @sergey-shandar, are you talking about someone who could take the published swagger spec and generate his own SDK from the spec? Does this mean that changing the swagger spec at this location effectively means publishing an SDK, and this is how we should treat it for all changes?

vrdmr · 2018-02-05T18:28:33Z

Hey @sergey-shandar,

Definetly this changes should be in the new API version but it doesn't make much sense to introduce SDK breaking changes for your customers.

Our swagger-based SDK is still in preview stage and contains many breaking changes. We released the preview SDK last month and are cleaning up the Specs before we can promote the SDK to a stable version.

Please let me know if you have any other questions.

cc: @marstr, @AnatoliB, @D1v38om83r

AnatoliB · 2018-02-06T19:02:24Z

Hi @sergey-shandar,

We need to close on this soon, as we are trying to fix all the swagger validation errors within the next few days, so I will appreciate your help very much.

Please note that there are multiple other PRs with similar changes coming from our team, and some of them have already been committed.

My previous point was that this may not be a breaking change in our context. But here is another question: what if we do want to make a breaking change? Is this not allowed under any circumstances, or there is an authority that could actually make this call? What I observed previously was that a reviewer would just point out a potential breaking change, and then approve the PR after the author acknowledges the issue. So, is this decision up to the Automation team?

sergey-shandar · 2018-02-06T21:05:38Z

Hi @AnatoliB. We assume that if RestAPI is published before as stable then people have freedom to generate stable API for any programming language. Including Microsoft official SDKs, for example for Python, Ruby, JavaScript, Go etc. For example, there's an official Microsoft stable NuGet package https://www.nuget.org/packages/Microsoft.Azure.Management.Automation/2.0.4 which is using the same API version 2015-10-03. So making changes in this swagger file will introduce SDK breaking changes for stable SDK for the same API version. We are trying hard to eliminate such breaking changes.

sergey-shandar

Since this API version is used in stable SDKs (NuGet 2.0.4), I would suggest not to make the SDK breaking changes. In particular, making properties optional are SDK breaking changes.

sergey-shandar · 2018-02-06T21:24:07Z

@AnatoliB

My previous point was that this may not be a breaking change in our context. But here is another question: what if we do want to make a breaking change?

It's always better to make SDK and server side breaking changes in a new API version if the current one is already publicly available.

Is this not allowed under any circumstances, or there is an authority that could actually make this call?

There could be some exception, for example, bugs in API etc.

What I observed previously was that a reviewer would just point out a potential breaking change, and then approve the PR after the author acknowledges the issue. So, is this decision up to the Automation team?

At the end, it's your service customers who may suffer. We can still approve this but we have to be sure that you understand all consequences. Is there a strong reason why you would like to introduce SDK breaking changes in the same API version?

vrdmr · 2018-02-06T22:59:15Z

The changes done in this PR reflect the Service and we are updating the specification to keep it updated. Also,

For example, there's an official Microsoft stable NuGet package https://www.nuget.org/packages/Microsoft.Azure.Management.Automation/2.0.4 which is using the same API version 2015-10-03. So making changes in this swagger file will introduce SDK breaking changes for stable SDK for the same API version.

The 2.0.4 API was generated from Hyak from the last PR done in SDK/Master.

vrdmr · 2018-02-08T20:25:14Z

Ping. Can you please take a look at this PR? Thanks.

sergey-shandar · 2018-02-12T19:12:41Z

@vrdmr we need ARM approval for this. @ravbhatnagar could you have a look, it's really simple change.

ravbhatnagar · 2018-02-15T20:50:20Z

Looks good.

vrdmr · 2018-02-15T21:50:28Z

@sergey-shandar Could you please approve and merge this change. Thanks.

AutorestCI · 2018-02-16T18:33:33Z

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#1959

AutorestCI · 2018-02-16T18:35:35Z

Automation for azure-sdk-for-go

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

Command: profileBuilder -s list -l ./profiles/2017-03-09/defintion.txt -name 2017-03-09
Finished with return code 127
and output:

/bin/sh: 1: profileBuilder: not found

lmazuel · 2018-02-21T21:45:56Z

@AutorestCI rebuild azure-sdk-for-python

AnatoliB added 3 commits January 26, 2018 15:38

Make 'properties' not required for PATCH on AutomationAccount_Update

ee68ef7

Make 'name' not required for PATCH on 'Webhook_Update'

d2e455c

Add x-ms-azure-resource=true to Webhook definition

b6acc77

azuresdkci assigned sergey-shandar Jan 30, 2018

AutorestCI mentioned this pull request Jan 30, 2018

Automatic PR of restapi_auto_AnatoliB_anatolib/FixAutomationSpecErrors into dev Azure/azure-sdk-for-go#1011

Closed

AutorestCI mentioned this pull request Jan 30, 2018

Automatic PR of restapi_auto_AnatoliB_anatolib/FixAutomationSpecErrors into master Azure/azure-sdk-for-python#1849

Closed

sergey-shandar added the potential-sdk-breaking-change label Jan 30, 2018

D1v38om83r approved these changes Jan 30, 2018

View reviewed changes

sergey-shandar suggested changes Feb 1, 2018

View reviewed changes

sergey-shandar suggested changes Feb 6, 2018

View reviewed changes

sergey-shandar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Feb 6, 2018

vrdmr mentioned this pull request Feb 13, 2018

Adding examples and fixing the DSC related lint issues #2415

Merged

10 tasks

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Feb 15, 2018

ravbhatnagar removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Feb 15, 2018

sergey-shandar approved these changes Feb 16, 2018

View reviewed changes

sergey-shandar merged commit ffa3d76 into Azure:master Feb 16, 2018

AutorestCI mentioned this pull request Feb 16, 2018

[AutoPR automation/resource-manager] Fix Automation swagger spec errors in the AutomationAccount and Webhook areas Azure/azure-sdk-for-python#2000

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Automation swagger spec errors in the AutomationAccount and Webhook areas #2360

Fix Automation swagger spec errors in the AutomationAccount and Webhook areas #2360

AnatoliB commented Jan 30, 2018 •

edited by azuresdkci

Loading

azuresdkciprbot commented Jan 30, 2018

AutorestCI commented Jan 30, 2018

AutorestCI commented Jan 30, 2018

sergey-shandar commented Jan 30, 2018

AnatoliB commented Jan 30, 2018

sergey-shandar commented Jan 31, 2018 •

edited

Loading

marstr commented Jan 31, 2018 •

edited

Loading

vrdmr commented Jan 31, 2018 •

edited

Loading

sergey-shandar left a comment •

edited

Loading

AnatoliB commented Feb 1, 2018

vrdmr commented Feb 5, 2018 •

edited

Loading

AnatoliB commented Feb 6, 2018

sergey-shandar commented Feb 6, 2018 •

edited

Loading

sergey-shandar left a comment •

edited

Loading

sergey-shandar commented Feb 6, 2018 •

edited

Loading

vrdmr commented Feb 6, 2018

vrdmr commented Feb 8, 2018

sergey-shandar commented Feb 12, 2018

ravbhatnagar commented Feb 15, 2018

vrdmr commented Feb 15, 2018

AutorestCI commented Feb 16, 2018 •

edited

Loading

AutorestCI commented Feb 16, 2018 •

edited

Loading

lmazuel commented Feb 21, 2018

Fix Automation swagger spec errors in the AutomationAccount and Webhook areas #2360

Fix Automation swagger spec errors in the AutomationAccount and Webhook areas #2360

Conversation

AnatoliB commented Jan 30, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

azuresdkciprbot commented Jan 30, 2018

AutorestCI commented Jan 30, 2018

AutorestCI commented Jan 30, 2018

sergey-shandar commented Jan 30, 2018

AnatoliB commented Jan 30, 2018

sergey-shandar commented Jan 31, 2018 • edited Loading

marstr commented Jan 31, 2018 • edited Loading

vrdmr commented Jan 31, 2018 • edited Loading

sergey-shandar left a comment • edited Loading

Choose a reason for hiding this comment

AnatoliB commented Feb 1, 2018

vrdmr commented Feb 5, 2018 • edited Loading

AnatoliB commented Feb 6, 2018

sergey-shandar commented Feb 6, 2018 • edited Loading

sergey-shandar left a comment • edited Loading

Choose a reason for hiding this comment

sergey-shandar commented Feb 6, 2018 • edited Loading

vrdmr commented Feb 6, 2018

vrdmr commented Feb 8, 2018

sergey-shandar commented Feb 12, 2018

ravbhatnagar commented Feb 15, 2018

vrdmr commented Feb 15, 2018

AutorestCI commented Feb 16, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Feb 16, 2018 • edited Loading

Automation for azure-sdk-for-go

lmazuel commented Feb 21, 2018

AnatoliB commented Jan 30, 2018 •

edited by azuresdkci

Loading

sergey-shandar commented Jan 31, 2018 •

edited

Loading

marstr commented Jan 31, 2018 •

edited

Loading

vrdmr commented Jan 31, 2018 •

edited

Loading

sergey-shandar left a comment •

edited

Loading

vrdmr commented Feb 5, 2018 •

edited

Loading

sergey-shandar commented Feb 6, 2018 •

edited

Loading

sergey-shandar left a comment •

edited

Loading

sergey-shandar commented Feb 6, 2018 •

edited

Loading

AutorestCI commented Feb 16, 2018 •

edited

Loading

AutorestCI commented Feb 16, 2018 •

edited

Loading