Skip to content

Conversation

@chandrasekarsrinivasan
Copy link
Contributor

@chandrasekarsrinivasan chandrasekarsrinivasan commented Jan 2, 2018

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

murilogiacometti and others added 6 commits November 21, 2017 16:10
* Video Swagger API: update example response json to remove special invalid special characters (Azure#2015)

* Add Video Search API Swagger

* Video Swagger API: update example response json to remove special invalid special characters

* take changes from azure-rest-spec-pr, make Microsoft.Subscription tenant level rp (Azure#1984)

* Fix bad merge. (Azure#2022)

* Add list apis for sql backup LTR policies & vaults (Azure#2006)

* Add list LTR vaults & policies

* Added pageable

* Change output folder for Search SDKs (Azure#2023)

* update output folders in readmes

* rename with bing in name

* App Model for Tumbling window trigger (Azure#2028)

* App model for tumbling window trigger

* Addressed CR comments

* Renamed retry to count in RetryPolicy

* swagger definition for 26 RFI connectors (Azure#2016)

* Updated OMS data plane C# namespace & output-folder (Azure#2021)

* Update Azure Batch enum to use "values" - also some documentation updates (Azure#2008)

* Enum fixes

* Improve DataDisks documentation

* Update NATPool port range documentation

* [Compute] Update Sku APIs (Azure#2034)

* Initial commit of 2017-12-01 compute.json

* Changes for new API version 2017-12-01 compute.json

* Add Compute SKU APIs

* Add swagger spec for spell check API (Azure#1997)

* Add swagger spec for spell check API

* Add Examples to spec

* Remove redundant forward slash in path. Specify array from body parameters.

* Fix oav errors

* Remove x-Bing_Apis-SDK

* Change spec to reflect what objects we want to expose in the response.

* Fix operationId name to avoid conflict with "SpellCheck" schema.

* Add X-BingApis-SDK back.

* Remove Unintentional change 

Packages.json change here was unintentionally added to this review
* Video Swagger API: update example response json to remove special invalid special characters (Azure#2015)

* Add Video Search API Swagger

* Video Swagger API: update example response json to remove special invalid special characters

* take changes from azure-rest-spec-pr, make Microsoft.Subscription tenant level rp (Azure#1984)

* Fix bad merge. (Azure#2022)

* Add 2017-11-01 folder in microsoft.network
* add iptags for publicip in 2017-11-01

* read me changes for 2017-11-01

* Add Python conf for Network 2017-11-01

* Complete Python conf for 2017-11-01
@sergey-shandar
Copy link
Contributor

@chandrasekarsrinivasan as far as I understood, the PR is a merge from Network-2017-11-01 to 'master'.
@Mazuel could you check why Python CI is failing?

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

@sergey-shandar sergey-shandar 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 Jan 2, 2018
@sergey-shandar
Copy link
Contributor

@ravbhatnagar a new Network API version (2017-11-01).

Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

Over the holidays, we made some changes to this repository. One of which is requiring that each API Version be categorized explicitly as either stable or preview. To do this, the API Version folder must be placed in a folder corresponding to the names mentioned above. You'll be able to see what I'm taking about here:
https://github.com/Azure/azure-rest-api-specs/tree/master/specification/network/resource-manager/Microsoft.Network

To read more about what happened over the holidays, please find our wiki page: https://github.com/Azure/azure-rest-api-specs/wiki/December-2017-Refactoring

Please move the files added in this PR to match the new format. Thanks!

edit: ordering

@chandrasekarsrinivasan
Copy link
Contributor Author

@sergey-shandar Yes

@chandrasekarsrinivasan
Copy link
Contributor Author

@marstr Moved files to stable folder

@azuresdkciprbot
Copy link

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/network/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 85
After the PR: Warning(s): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

Copy link
Member

@marstr marstr left a comment

Choose a reason for hiding this comment

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

My approval speaks only to the location of these changes. The actual changes in this PR are still subject to the normal process and @sergey-shandar's judgement.

@chandrasekarsrinivasan
Copy link
Contributor Author

@sergey-shandar Can you please help close on this?

@sergey-shandar
Copy link
Contributor

@ravbhatnagar could you review the merge PR?

@lmazuel
Copy link
Member

lmazuel commented Jan 10, 2018

@lmazuel tag myself to get notifications (@sergey-shandar you made a typo ;))

@chandrasekarsrinivasan chandrasekarsrinivasan force-pushed the chsrini_pulllatestfromMaster branch from ec26c7e to f30750a Compare January 10, 2018 20:52
@azuresdkciprbot
Copy link

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/network/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 85
After the PR: Warning(s): 0 Error(s): 85

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@ravbhatnagar
Copy link
Contributor

ravbhatnagar commented Jan 10, 2018

Reviewed this in person. The only feedback from ARM side was to make ipTagType property as an enum. It would improve the usability of the API and developers can deterministically code against it. Specially considering the case when new values will be supported by the service for this. Currently only 2 are supported but more will be supported in future. So it makes it hard for developers using the API know when new values get added or what new values are supported.
Making it an enum helps in both the cases where the supported values in an api-version will be known. These dont have to searched in the documentation. And then when new values are supported, they get added to a new api-version again improving the usability.
Moreover, the type does not have to change. "type" will still be string. And so service side wont have to change. example -


@chandrasekarsrinivasan is going to circle back with his team on whether they want to model this as an enum.

@chandrasekarsrinivasan
Copy link
Contributor Author

@ravbhatnagar - I had a discussion with my team members @avijitgupta and @murilogr. The advantages of having an enum is that we will have better usability and the disadvantages is that we will have new API versions when we have changes to it. Then, we discussed and we see that we will have additions to this. Hence, we decided not to go with enum.

Copy link
Contributor

@sergey-shandar sergey-shandar left a comment

Choose a reason for hiding this comment

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

LGTM.

@sergey-shandar sergey-shandar merged commit b0e4eae into Azure:master Jan 11, 2018
@sergey-shandar sergey-shandar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review labels Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants