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.

Sign up for GitHub

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

Jump to bottom

Add initial set for changes for ANT79 #4435

Merged
merged 2 commits into from
Nov 9, 2018
Merged

Add initial set for changes for ANT79 #4435

merged 2 commits into from
Nov 9, 2018

Conversation

naveedaz
Copy link
Contributor

@naveedaz naveedaz commented Nov 8, 2018

Latest improvements:

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

  • I have reviewed the documentation for the workflow.
  • Validation tools were run on swagger spec(s) and have all been fixed in this PR.
  • The OpenAPI Hub was used for checking validation status and next steps.

@openapi-portal-comment
Copy link

If you're a MSFT employee, click this link
to view this PR's validation status on our new OpenAPI Hub spec management tool.

@azuresdkci azuresdkci requested a review from dsgouda November 8, 2018 23:14
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#3764

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-ruby

A PR has been created for you:
Azure/azure-sdk-for-ruby#1885

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-js

A PR has been created for you:
Azure/azure-sdk-for-js#455

dsgouda
dsgouda suggested changes Nov 8, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Please rearrange the changes so that the diff is accurate and easier to review

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#3267

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#4064

@AutorestCI AutorestCI mentioned this pull request Nov 8, 2018
Merged
@AutorestCI
Copy link

AutorestCI commented Nov 8, 2018 •
edited
Loading

Automation for azure-sdk-for-java

A PR has been created for you:
Azure/azure-sdk-for-java#2581

@naveedaz
Copy link
Contributor Author

naveedaz commented Nov 9, 2018

@dsgouda We auto-generate the swagger files. For now I have re-arranged it. I'll look into whether we can maintain this order while generating for future PRs.
Please review these changes.

@dsgouda
Copy link
Contributor

dsgouda commented Nov 9, 2018

Thanks @naveedaz

dsgouda
dsgouda suggested changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Left a few comments

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/CommonDefinitions.json
@@ -686,6 +686,10 @@
"items": {
"type": "string"
}
},
"supportCredentials": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these changes being made to an existing stable version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to stable released versions must be approved by ARM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These changes are additive. We do not want to release a new API-version for small non-breaking changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, missed that. Since these are additive changes I would guess these are not breaking changes.

specification/web/resource-manager/Microsoft.Web/stable/2018-02-01/WebApps.json
@@ -1632,7 +1632,7 @@
}
}
},
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/config/virtualNetwork": {
"/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.Web/sites/{name}/networkConfig/virtualNetwork": {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be beyond the scope of this PR, but this spec has a few operations that define 404 error status codes explicitly. AutoRest does not support these today (we have an experimental extension to handle this if you really need it). Please remove these error status codes (or apply the extension) and regenerate the SDKs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda Thanks for calling out the 404 status codes. I will talk to the feature owners to figure out if we should remove the status codes or use the experimental extension. We will change that as a new PR.
Please share the link to the extension, I could not find it on https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md

Are you good with all the other changes? If so, please approve.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is a brief explanation

@dsgouda dsgouda added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 9, 2018
dsgouda
dsgouda approved these changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM

@dsgouda dsgouda merged commit 586159e into Azure:dev-web-Microsoft.Web-ANT79 Nov 9, 2018
salameer pushed a commit that referenced this pull request Jan 17, 2019
@naveedaz @salameer
Merge changes from release microsoft.web ant79 (#5048)
08c5d9f 
* Add initial set for changes for ANT79 (#4435)

* Add initial set for changes for ANT79

* Fix ordering of swift VNET APIs

* Add GeoRegion to DeletedSite Model (#4453)
TalluriAnusha pushed a commit to AsrOneSdk/azure-rest-api-specs that referenced this pull request Feb 6, 2019
@naveedaz @AnuTalluri
Merge changes from release microsoft.web ant79 (Azure#5048)
2a28a94 
* Add initial set for changes for ANT79 (Azure#4435)

* Add initial set for changes for ANT79

* Fix ordering of swift VNET APIs

* Add GeoRegion to DeletedSite Model (Azure#4453)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsgouda dsgouda dsgouda approved these changes

Assignees

@dsgouda dsgouda

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@naveedaz @azuresdkci @AutorestCI @dsgouda