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

[Don't Merge, Service rollout ~March/1/18 ] Swagger for new API changes 2017-12-01 #2207

Closed
wants to merge 25 commits into from

Conversation

revachauhan
Copy link

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

  • Swagger for new business model changes API
  • 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

@anuchandy
Copy link
Member

@jhendrixMSFT do you mind if i take this PR? [Reva previously opened another PR with the same features and I requested him to close that PR and open new one]

@jhendrixMSFT
Copy link
Member

@anuchandy Assigned to you.

@anuchandy anuchandy added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Jan 3, 2018
@anuchandy anuchandy requested a review from ravbhatnagar January 3, 2018 17:48
@anuchandy anuchandy changed the title Swagger for new API changes 2017-12-01 [Don't Merge] Swagger for new API changes 2017-12-01 Jan 5, 2018
@anuchandy
Copy link
Member

Talked to Reva and learned that the service will take one month to land in production so team don't want to publish the new API swagger as of now. The estimated date is March 1. Hence adding 'DONT MERGE' prefix to title.

@anuchandy
Copy link
Member

@revas89 it looks like you didn't add entry to readme file for the new api version, can you please do so?

@revachauhan
Copy link
Author

@anuchandy Made changes to readme.md file


``` yaml $(tag) == 'package-2017-12-01'
input-file:
- Microsoft.DBforPostgreSQL/2017-12-01-preview/postgresql.json
Copy link
Member

Choose a reason for hiding this comment

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

should this be Microsoft.DBforPostgreSQL/2017-12-01/postgresql.json?

Please also specify `--go-sdk-folder=<path to the root directory of your azure-sdk-for-go clone>`.

``` yaml $(tag) == 'package-2017-12-01' && $(go)
output-folder: $(go-sdk-folder)/services/postgresql/mgmt/2017-04-30-preview/postgresql
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above

@anuchandy
Copy link
Member

@revas89 thanks for readme update.. please see last two comments

Copy link
Contributor

@rohit-joy rohit-joy left a comment

Choose a reason for hiding this comment

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

Do not merge until our team decides it is time.

@anuchandy
Copy link
Member

@rohit-joy & @revas89 sure I've highlighted that in the PR title. Meantime please correct the readme file. Though there is new tag, it's still pointing to older preview api version.

@anuchandy
Copy link
Member

@revas89 thanks for updating the readme. Can you take a look and fix linter and model validation errors?

@@ -0,0 +1,19 @@
{
Copy link

Choose a reason for hiding this comment

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

I did not see anything about Microsoft.DBforMySQL, will it be included in this PR?

Copy link
Author

Choose a reason for hiding this comment

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

i have added mysql.json and examples in this PR.

@yukozh
Copy link

yukozh commented Jan 10, 2018

Hi @revas89, I didn't see anything about Microsoft.DBforMySQL, will you add it in this PR?

"dtu": 50,
"storageMB": 51200,
"id": "PGSQLB50"
"Edition": "Basic",
Copy link

Choose a reason for hiding this comment

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

In my imagination, these fields are in lower case.

"id": "PGSQLB100"
"Edition": "Basic",
"HardwareGeneration": "Gen4",
"Id": "PGSQL_B_Gen4_2",
Copy link

Choose a reason for hiding this comment

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

I did not see the PGSQL_ prefix in our tier id. When I called the GetTiers API, it just returned B_Gen4_2

Copy link
Author

Choose a reason for hiding this comment

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

I am able to see that when I am running it on C# generated SDK

Copy link
Author

Choose a reason for hiding this comment

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

yes that is right, I will change in my example.

"type": "integer",
"description": "Database throughput unit associated with the service level objective"
"description": "VCore associated with the service level objective"
Copy link

Choose a reason for hiding this comment

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

Should it be vCore? Not VCore?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@@ -19,7 +19,7 @@
"type": "Microsoft.DBforMySQL/servers",
"location": "onebox",
Copy link

Choose a reason for hiding this comment

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

Should onebox be other real region be better?

@ravbhatnagar
Copy link
Contributor

review feedback from the in person meeting. @revas89 - Please incorporate and schedule a follow up to go over the remaining changes we could not cover.
-check camelCase for all properties
-storageMB - does it have max or min
-createMode to be an enum - i think it already is. Wasnt showing up in the commit we were first reviewing. please confirm
-performanceTier API being deprecated in GA api-version. Service will still support the API for a while and notify customers.
-sourceServerId needs to be the fully qualified ARm resource ID. Reva to check
-SKU contract not being followed. Sent link. Reva will check how SQL is doing it.

  • performancetier->storageinMBMax and min are two separate prop instead of one on the server resource. On the server resource is called storageinMB. Is there a case for having consistent experience?

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

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@ravbhatnagar
Copy link
Contributor

Signing off as this was reviewed in person again after the changes were made based on feedback from earlier review.

@ravbhatnagar ravbhatnagar 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 labels Jan 16, 2018
@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/mysql/resource-manager/readme.md
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 0 Error(s): 0

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@anuchandy anuchandy changed the title [Don't Merge] Swagger for new API changes 2017-12-01 [Don't Merge, Service rollout ~March/1/18 ] Swagger for new API changes 2017-12-01 Jan 19, 2018
@jhendrixMSFT
Copy link
Member

@AutorestCI rebuild azure-sdk-for-go

@AutorestCI
Copy link

AutorestCI commented Mar 16, 2018

Automation for azure-sdk-for-go

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

@QingqingYuan
Copy link
Contributor

@revas89 Please abandon this PR. We've restructured our swagger files. This is not needed any more.

@anuchandy
Copy link
Member

closing this based on @QingqingYuan comment.

@anuchandy anuchandy closed this Apr 13, 2018
@AutorestCI
Copy link

AutorestCI commented Apr 13, 2018

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

@AutorestCI
Copy link

AutorestCI commented Apr 13, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Apr 13, 2018

Automation for azure-sdk-for-python

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants