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

matthchr · 2017-11-14T16:52:54Z

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.

matthchr · 2017-11-14T16:54:50Z

@xingwu1 @darylmsft - Please take a look

@smith1511 - Take a look at the commit for NATPool docs

xingwu1 · 2017-11-14T18:02:45Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

Don't need specify "name" if not override the enum name.

I will fix this

xingwu1 · 2017-11-14T18:07:11Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

currentUser vs currentuser in the "description"? None of it is good.

xingwu1 · 2017-11-14T18:10:37Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

Why empty here?

xingwu1 · 2017-11-14T18:11:30Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

inconsistent. Some places use all lower cases. Here is different.

xingwu1 · 2017-11-14T19:16:29Z

Major issue: I tried this spec with different languages. C# looks good. However, neither java or python honor this extension. So we can't approve this PR until major languages support it.

darylmsft · 2017-11-14T22:16:40Z

I think/hope Xing means "cannot approve..." right?

matthchr · 2017-11-14T22:58:28Z

@xingwu1 I knew that Python didn't honor this -- can you file (or help me file) a GitHub issue for Java like the ones I filed for node and python?

I already have commitment from @amarzavery to fix Node.js this month, and I spoke with @annatisch who says that we should be able to have the fix for Python within relatively short order as well.

Given that this also fixes the C# SDK and is just generally more correct it seemed better to move and get the tooling fixed by the time we regenerate new SDKs (which probably wont be for a while).

mcardosos · 2017-11-15T01:47:11Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

noAction [](start = 9, length = 8)

noaction :D

mcardosos

Examples should also be updated

mcardosos · 2017-11-15T02:38:44Z

@matthchr many of this changes have to do with documentation, but some other are because the swagger was not describing correctly the API, am I right?

matthchr · 2017-11-15T03:01:55Z

@mcardosos - Basically yes - the Swagger was using a camelCased name because we had to in order to get well generated code. But the service actually doesn't return the text camelCase, it returns it all lowercase. BUT in all the generated languages they were case insensitive here anyway so it didn't matter.

Newtonsoft 10 came out and changed that in C# (now sometimes it IS case sensitive), so we needed to update the spec to more accurately match the behavior of the service (while still keeping the nice naming we had before by using the values extension to x-ms-enum.)

azuresdkciprbot · 2017-11-15T17:23:35Z

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/batch/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

matthchr · 2017-11-17T17:46:25Z

@mcardosos I updated the samples -- take a look again?

mcardosos · 2017-11-18T01:43:35Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

terminateJob [](start = 9, length = 12)

terminatejob

mcardosos · 2017-11-18T01:47:29Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

waitingForStartTask [](start = 26, length = 19)

waitingforstarttask

mcardosos · 2017-11-18T01:48:35Z

specification/batch/data-plane/Microsoft.Batch/2017-09-01.6.0/BatchService.json

taskCompletion [](start = 13, length = 14)

taskcompletion

mcardosos · 2017-11-18T01:51:54Z

@matthchr Awesome
Thanks for updating the examples. There are just some tiny details with camel case and it should be good enough to merge :D

azuresdkciprbot · 2017-11-20T17:55:39Z

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/batch/data-plane/readme.md
Before the PR: Warning(s): 0 Error(s): 1
After the PR: Warning(s): 0 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

matthchr · 2017-11-20T18:22:42Z

Thanks @mcardosos for the good catches! I think things look okay now.

mcardosos · 2017-11-20T19:08:39Z

Great!
There is a linter error in the swagger, but as it was not introduced on this PR, this PR is approved. Just mentioning this for awareness.

AutorestCI · 2017-11-20T19:12:25Z

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

AutorestCI · 2017-11-20T19:12:37Z

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

AutorestCI · 2017-11-20T19:13:14Z

Azure/azure-sdk-for-python#1635

* Video Swagger API: update example response json to remove special invalid special characters (#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 (#1984) * Fix bad merge. (#2022) * Add list apis for sql backup LTR policies & vaults (#2006) * Add list LTR vaults & policies * Added pageable * Change output folder for Search SDKs (#2023) * update output folders in readmes * rename with bing in name * App Model for Tumbling window trigger (#2028) * App model for tumbling window trigger * Addressed CR comments * Renamed retry to count in RetryPolicy * swagger definition for 26 RFI connectors (#2016) * Updated OMS data plane C# namespace & output-folder (#2021) * Update Azure Batch enum to use "values" - also some documentation updates (#2008) * Enum fixes * Improve DataDisks documentation * Update NATPool port range documentation * [Compute] Update Sku APIs (#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 (#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

* [Network-2017-11-01] Pull latest from current (#2039) * Video Swagger API: update example response json to remove special invalid special characters (#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 (#1984) * Fix bad merge. (#2022) * Add list apis for sql backup LTR policies & vaults (#2006) * Add list LTR vaults & policies * Added pageable * Change output folder for Search SDKs (#2023) * update output folders in readmes * rename with bing in name * App Model for Tumbling window trigger (#2028) * App model for tumbling window trigger * Addressed CR comments * Renamed retry to count in RetryPolicy * swagger definition for 26 RFI connectors (#2016) * Updated OMS data plane C# namespace & output-folder (#2021) * Update Azure Batch enum to use "values" - also some documentation updates (#2008) * Enum fixes * Improve DataDisks documentation * Update NATPool port range documentation * [Compute] Update Sku APIs (#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 (#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 * Add 2017-11-01 folder in microsoft.network (#2019) * Video Swagger API: update example response json to remove special invalid special characters (#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 (#1984) * Fix bad merge. (#2022) * Add 2017-11-01 folder in microsoft.network * Add Ip Tags into Swagger (#2145) * 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 * Updated LB InboundNatPool API (#2150) * Revert "[Network-2017-11-01] Pull latest from current (#2039)" This reverts commit a0257f9. * Moving to stable folder to account for refactoring changes in dec 2017

azuresdkci assigned mcardosos Nov 14, 2017

matthchr mentioned this pull request Nov 14, 2017

Fix build issue and enum casing bug for Azure Batch Azure/azure-sdk-for-net#3861

Merged

8 tasks

xingwu1 reviewed Nov 14, 2017

View reviewed changes

matthchr force-pushed the feature/batch-enum-fixes branch from 4a3154d to 87a95cc Compare November 14, 2017 23:46

mcardosos reviewed Nov 15, 2017

View reviewed changes

mcardosos suggested changes Nov 15, 2017

View reviewed changes

matthchr force-pushed the feature/batch-enum-fixes branch 2 times, most recently from 9b56f85 to 8400f17 Compare November 15, 2017 17:19

mcardosos reviewed Nov 18, 2017

View reviewed changes

matthchr added 3 commits November 20, 2017 09:50

Enum fixes

04c4e14

Improve DataDisks documentation

7b370f0

Update NATPool port range documentation

cd9b7bf

matthchr force-pushed the feature/batch-enum-fixes branch from 8400f17 to cd9b7bf Compare November 20, 2017 17:51

mcardosos approved these changes Nov 20, 2017

View reviewed changes

mcardosos merged commit 75ff961 into Azure:current Nov 20, 2017

AutorestCI mentioned this pull request Nov 20, 2017

Automatic PR from RestAPI-PR2008 Azure/azure-sdk-for-python#1635

Closed

matthchr deleted the feature/batch-enum-fixes branch November 20, 2017 19:24

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

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

Uh oh!

Conversation

matthchr commented Nov 14, 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!

matthchr commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xingwu1 commented Nov 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darylmsft commented Nov 14, 2017

Uh oh!

matthchr commented Nov 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcardosos left a comment

Choose a reason for hiding this comment

Uh oh!

mcardosos commented Nov 15, 2017

Uh oh!

matthchr commented Nov 15, 2017

Uh oh!

azuresdkciprbot commented Nov 15, 2017

Uh oh!

matthchr commented Nov 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcardosos commented Nov 18, 2017

Uh oh!

azuresdkciprbot commented Nov 20, 2017

Uh oh!

matthchr commented Nov 20, 2017

Uh oh!

mcardosos commented Nov 20, 2017

Uh oh!

AutorestCI commented Nov 20, 2017

Uh oh!

AutorestCI commented Nov 20, 2017

Uh oh!

AutorestCI commented Nov 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

matthchr commented Nov 14, 2017 •

edited by azuresdkci

Loading

xingwu1 commented Nov 14, 2017 •

edited

Loading