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

New swagger for 2019-08-01 API #7660

Merged
merged 16 commits into from
Nov 7, 2019
Merged

New swagger for 2019-08-01 API #7660

merged 16 commits into from
Nov 7, 2019

Conversation

leonardbf
Copy link
Contributor

@leonardbf leonardbf commented Oct 29, 2019

For R6 release of RP with new API 2019-08-01
First commit contains previous 2019-07-01 as 2019-08-01.
Amendments to 08 baseline version

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.

ARM API Review Checklist

  • Service team MUST add the "WaitForARMFeedback" label if the management plane API changes fall into one of the below categories.
  • adding/removing APIs.
  • adding/removing properties.
  • adding/removing API-version.
  • adding a new service in Azure.

Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs.

  • If you are blocked on ARM review and want to get the PR merged urgently, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
    Please follow the link to find more details on API review process.

leonardbf and others added 4 commits October 25, 2019 16:23
For R6.
Initial version, identical to 2019-07-01 as baseline.
For R6
Amendments to 08 baseline version
 - corrections to dates
 - R6 specific alterations
ANF-327 New swagger for 2019-08-01 API
@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 29, 2019

In Testing, Please Ignore

[Logs] (Generated from 3f16eea, Iteration 11)

Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Failed .NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
Warning JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
  • Warning @azure/arm-netapp [Logs]
In-Progress Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Failed Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
  • Failed netapp/resource-manager/v2017_08_15 [Logs]
  • Failed netapp/resource-manager/v2019_05_01 [Logs]
  • Failed netapp/resource-manager/v2019_06_01 [Logs]
  • Failed netapp/resource-manager/v2019_08_01 [Logs]

@AutorestCI
Copy link

AutorestCI commented Oct 29, 2019

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#6288

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@leonardbf
Copy link
Contributor Author

leonardbf commented Oct 29, 2019

Assistance required:

  1. It seems prettify checks have been introduced/changed. It's not clear what the format error is. Running npm install as per the instructions here https://dev.azure.com/azure-sdk/public/_build/results?buildId=173322&view=logs&jobId=b56e9678-4dad-5642-289c-5530cc4dad11 gives "npm ERR! [email protected] postinstall: scripts/switch-to-preproduction.sh". Despite this, I can actually run 'prettier' locally, which does perform some reformatting of the jsons, however I suspect it is not seeing a correct config, because it still fails in this pipeline.
  2. The breaking change here https://dev.azure.com/azure-sdk/public/_build/results?buildId=173321&view=logs&j=08b5038d-1e84-5877-88da-859df8312b69 is something that intermittently occurs when we push a new API even when no change has been made. It is always on the same location but does not always occur. It would be great to get some explanation.
    Thanks.

@jhendrixMSFT jhendrixMSFT added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Oct 29, 2019
@jhendrixMSFT
Copy link
Member

@NullMDR are you the right person to help with these issues?

@AutorestCI
Copy link

AutorestCI commented Oct 30, 2019

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#8488

@leonardbf
Copy link
Contributor Author

leonardbf commented Oct 30, 2019

@NullMDR are you the right person to help with these issues?

I see #7666 (comment) was produced and this resolves the problem I had running 'npm run prettier' locally. However the resulting json still fails the check when run in the pipeline. So while the first part of my first issue above is resolved the last part of the first issue and the second issue are still relevant.

@PhoenixHe-NV
Copy link
Contributor

PhoenixHe-NV commented Nov 1, 2019

@leonardbf Please rebase on the latest master and run

npm install; npm run prettier -- --write "specification/netapp/**/*.json"

@qiaozha
Copy link
Member

qiaozha commented Nov 4, 2019

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@qiaozha
Copy link
Member

qiaozha commented Nov 4, 2019

@leonardbf @raych1 Currently, BreakingChange doesn't support to put "parameters" being parrellel to methods like "get", "put", "post". Because a method should be defined as an json object, but here parameters was defined as an array. which cause json parse error. For example, from line 70 at specification/netapp/resource-manager/Microsoft.NetApp/stable/2019-05-01/netapp.json.
The reason why you don't change this part how this time the error come out was because Previously this file was forcely merged, but you've changed other part this time. The BreakingChange Tool will check the whole file and report it again.
This is an acknowledged bug for BreakingChange Tool and my suggestion is to move the "parameter" block into the methods separately like this example move "parameters" block at line 70 into "post" block at line 91.
Otherwise. you need to merge it forcely like previously. but in this way, before this error being fixed, each time you change this file, it will come out.
This kind of code style can be seen in several places.

@leonardbf
Copy link
Contributor Author

@leonardbf @raych1 Currently, BreakingChange doesn't support to put "parameters" being parrellel to methods like "get", "put", "post". Because a method should be defined as an json object, but here parameters was defined as an array. which cause json parse error. For example, from line 70 at specification/netapp/resource-manager/Microsoft.NetApp/stable/2019-05-01/netapp.json.
The reason why you don't change this part how this time the error come out was because Previously this file was forcely merged, but you've changed other part this time. The BreakingChange Tool will check the whole file and report it again.
This is an acknowledged bug for BreakingChange Tool and my suggestion is to move the "parameter" block into the methods separately like this example move "parameters" block at line 70 into "post" block at line 91.
Otherwise. you need to merge it forcely like previously. but in this way, before this error being fixed, each time you change this file, it will come out.
This kind of code style can be seen in several places.

Ok, thanks for the update. Since this has been pushed before with this present would it be acceptable to do so again? We would address this problem in our API 2019-10-01 which we expect to push at the end of November. The 2019-08-01 is needed for the RP that went to production last week.

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

I added a couple of questions. Can you take a look?

@qiaozha
Copy link
Member

qiaozha commented Nov 5, 2019

@leonardbf @raych1 Currently, BreakingChange doesn't support to put "parameters" being parrellel to methods like "get", "put", "post". Because a method should be defined as an json object, but here parameters was defined as an array. which cause json parse error. For example, from line 70 at specification/netapp/resource-manager/Microsoft.NetApp/stable/2019-05-01/netapp.json.
The reason why you don't change this part how this time the error come out was because Previously this file was forcely merged, but you've changed other part this time. The BreakingChange Tool will check the whole file and report it again.
This is an acknowledged bug for BreakingChange Tool and my suggestion is to move the "parameter" block into the methods separately like this example move "parameters" block at line 70 into "post" block at line 91.
Otherwise. you need to merge it forcely like previously. but in this way, before this error being fixed, each time you change this file, it will come out.
This kind of code style can be seen in several places.

Ok, thanks for the update. Since this has been pushed before with this present would it be acceptable to do so again? We would address this problem in our API 2019-10-01 which we expect to push at the end of November. The 2019-08-01 is needed for the RP that went to production last week.

@yungezz Catherine, this PR BreakingChange error need to be force merged. Could you pls take a look into it? Thanks

@yungezz
Copy link
Member

yungezz commented Nov 5, 2019

@leonardbf @raych1 Currently, BreakingChange doesn't support to put "parameters" being parrellel to methods like "get", "put", "post". Because a method should be defined as an json object, but here parameters was defined as an array. which cause json parse error. For example, from line 70 at specification/netapp/resource-manager/Microsoft.NetApp/stable/2019-05-01/netapp.json.
The reason why you don't change this part how this time the error come out was because Previously this file was forcely merged, but you've changed other part this time. The BreakingChange Tool will check the whole file and report it again.
This is an acknowledged bug for BreakingChange Tool and my suggestion is to move the "parameter" block into the methods separately like this example move "parameters" block at line 70 into "post" block at line 91.
Otherwise. you need to merge it forcely like previously. but in this way, before this error being fixed, each time you change this file, it will come out.
This kind of code style can be seen in several places.
Ok, thanks for the update. Since this has been pushed before with this present would it be acceptable to do so again? We would address this problem in our API 2019-10-01 which we expect to push at the end of November. The 2019-08-01 is needed for the RP that went to production last week.

@yungezz Catherine, this PR BreakingChange error need to be force merged. Could you pls take a look into it? Thanks

got it. once ARM review signed off, I can do merge.

@leonardbf
Copy link
Contributor Author

I added a couple of questions. Can you take a look?

@majastrz I responded to the questions and hope this can be signed off soon. Thanks.

@majastrz majastrz added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMReviewInProgress WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 5, 2019
Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

Looks good from ARM side.

@jhendrixMSFT
Copy link
Member

@leonardbf just to confirm, this API version is now live?

@leonardbf
Copy link
Contributor Author

@leonardbf just to confirm, this API version is now live?

Yep, this went live last week.

@qiaozha
Copy link
Member

qiaozha commented Nov 7, 2019

/azp run

1 similar comment
@qiaozha
Copy link
Member

qiaozha commented Nov 7, 2019

/azp run

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