Make ContainerInstance a standalone SDK by yolocs · Pull Request #1480 · Azure/azure-rest-api-specs

yolocs · 2017-07-27T18:52:37Z

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.

msftclas · 2017-07-27T18:52:40Z

@yolo3301,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

lmazuel · 2017-07-27T19:31:29Z

specification/containerinstance/resource-manager/readme.md

+  # override the default output folder
+  output-folder: $(output-folder)/python
+  license-header: MICROSOFT_MIT_NO_VERSION
+  payload-flattening-threshold: 2


Add namespace: azure.mgmt.containerinstance

I will override for now in my conf, but would be nice directly here

Added. Thanks!

lmazuel

Incorrect tag

lmazuel · 2017-07-27T19:38:34Z

specification/containerinstance/resource-manager/readme.md

+```
+
+
+### Tag: package-container-instance-2017-08-preview


Blocking: this tag is incorrect and should be package-2017-08-preview in the three places used

lmazuel · 2017-07-27T19:43:45Z

Title is incorrect:
"title": "Microsoft Container Instance Management Client"
should not contain Microsoft. This field is used to generate the client name, it's not just documentation.

I will override it for now in Python. @olydis for guidance.

lmazuel · 2017-07-27T19:45:16Z

I approved the tag fix, because it allows me to use the MD, but title is still to be fixed.

yolocs · 2017-07-27T19:50:30Z

@lmazuel Title updated

lmazuel · 2017-07-27T19:52:08Z

Python PR: Azure/azure-sdk-for-python#1330
I'm starting the Python review to see if there is more to change at the Swagger level

azuresdkciprbot · 2017-07-27T19:58:42Z

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

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

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

lmazuel · 2017-07-27T20:02:25Z

Need guidance to confirm list best practices ( @olydis @fearthecowboy @sergey-shandar @veronicagg @amarzavery please :)), but in my understanding:

List by subscription should be called "list" (not "list_all" as currently)
List by RG should be called "list_by_resource_group" (not "list" as currently)

olydis · 2017-07-27T20:22:56Z

@lmazuel Absolutely correct, thanks for catching that. Furthermore:

shouldn't tail be an integer instead? https://github.com/yolo3301/azure-rest-api-specs/blob/dd1b3f3d090c21d4e835de87c038b309f2af392e/specification/containerinstance/resource-manager/Microsoft.ContainerInstance/2017-08-01-preview/containerInstance.json#L278
I heard that your RP doesn't support updates. But then there is ContainerGroups_CreateOrUpdate. Which is correct?

yolocs · 2017-07-27T20:42:46Z

@olydis Update is going to be supported in the near future in this API.

azuresdkciprbot · 2017-07-27T21:05:27Z

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

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

AutoRest Linter Guidelines | AutoRest Linter Issues

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

olydis · 2017-07-27T21:16:43Z

specification/compute/resource-manager/readme.md

 ```

-### Tag: package-container-instance-2017-08-preview
-


thanks for removing 👍

olydis · 2017-07-27T21:25:38Z

synced with @dsgouda, awaiting his signoff

yolocs · 2017-07-27T21:27:57Z

@olydis @lmazuel @dsgouda Thanks guys!

dsgouda · 2017-07-27T21:39:56Z

Signing off, thanks @olydis , @lmazuel

AutorestCI · 2017-07-27T21:45:00Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-07-27T21:45:15Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-07-27T21:45:20Z

No modification for AutorestCI/azure-sdk-for-node

Make ContainerInstance a standalone SDK

b149386

msftclas added the cla-not-required label Jul 27, 2017

azuresdkci assigned dsgouda Jul 27, 2017

lmazuel reviewed Jul 27, 2017

View reviewed changes

added namespace for python package

69ab673

lmazuel requested changes Jul 27, 2017

View reviewed changes

corrected the tags in readme

4073adc

lmazuel approved these changes Jul 27, 2017

View reviewed changes

adjusted containerInstance swagger title

dd1b3f3

adjusted api naming

aa1318f

olydis reviewed Jul 27, 2017

View reviewed changes

olydis approved these changes Jul 27, 2017

View reviewed changes

olydis merged commit 777755f into Azure:current Jul 27, 2017

ScottHolden mentioned this pull request Aug 14, 2017

Container instances/groups management client Azure/azure-sdk-for-net#3580

Closed

7 tasks

Conversation

yolocs commented Jul 27, 2017 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

msftclas commented Jul 27, 2017

Uh oh!

lmazuel Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

yolocs Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

lmazuel left a comment

Choose a reason for hiding this comment

Uh oh!

lmazuel Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

yolocs Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

lmazuel commented Jul 27, 2017

Uh oh!

lmazuel commented Jul 27, 2017

Uh oh!

yolocs commented Jul 27, 2017

Uh oh!

lmazuel commented Jul 27, 2017

Uh oh!

azuresdkciprbot commented Jul 27, 2017

Uh oh!

lmazuel commented Jul 27, 2017

Uh oh!

olydis commented Jul 27, 2017

Uh oh!

yolocs commented Jul 27, 2017

Uh oh!

azuresdkciprbot commented Jul 27, 2017

Uh oh!

olydis Jul 27, 2017

Choose a reason for hiding this comment

Uh oh!

olydis commented Jul 27, 2017

Uh oh!

yolocs commented Jul 27, 2017

Uh oh!

dsgouda commented Jul 27, 2017

Uh oh!

AutorestCI commented Jul 27, 2017

Uh oh!

AutorestCI commented Jul 27, 2017

Uh oh!

AutorestCI commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

yolocs commented Jul 27, 2017 •

edited by azuresdkci

Loading