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

Azure stack deployment resource provider admin open api specification #7561

Merged
merged 4 commits into from
Nov 6, 2019

Conversation

viananth
Copy link
Member

@viananth viananth commented Oct 21, 2019

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.

@viananth viananth requested review from sarathys and bganapa October 21, 2019 18:52
@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Oct 21, 2019

In Testing, Please Ignore

[Logs] (Generated from 710a9c7, Iteration 6)

No readme.md specification configuration files were found that are associated with the files modified in this pull request.

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Oct 21, 2019

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@lirenhe
Copy link
Member

lirenhe commented Oct 22, 2019

@viananth, please fix those code check errors in Avocado and ModelValidation.

@lirenhe lirenhe added Changes Required WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required and removed Changes Required labels Oct 22, 2019
@lirenhe
Copy link
Member

lirenhe commented Oct 24, 2019

@viananth, please fix those code check errors in Avocado and ModelValidation.

@lirenhe We are having an issue with representing JToken format. Could you please help how we should specify in the spec?
error : operationId: ActionPlanOperations_Get scenario: Gets the specified action plan operation source: response responseCode: '200' severity: 0 code: INVALID_TYPE details: code: INVALID_TYPE params: - string - object message: Expected type string but found type object path: properties/outputs title: '#/definitions/ActionPlanOperationAdminProperties/properties/outputs' description: The action plan operation outputs in JToken format position: line: 162 column: 20 url: >- specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/ActionPlanOperation.json directives: {} jsonUrl: >- specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/examples/ActionPlanOperations/Get.json jsonPath: $.properties.outputs

@viananth, I will take a look

@Azure Azure deleted a comment from viananth Oct 24, 2019
@lirenhe
Copy link
Member

lirenhe commented Oct 24, 2019

@viananth, wrong click to remove your comments. Could you leave this issue for now and fix other problems first?

@bganapa
Copy link
Member

bganapa commented Oct 24, 2019

@lirenhe I posted the question on Teams Channel....
Is there a precedence on how to specify JToken parameter in swagger object model?We have specified the type as string in for the JToken param. The Oav validate-example fails as the example is not matching to the defined type string while it is expecting object

@lirenhe
Copy link
Member

lirenhe commented Oct 24, 2019

@lirenhe I posted the question on Teams Channel....
Is there a precedence on how to specify JToken parameter in swagger object model?We have specified the type as string in for the JToken param. The Oav validate-example fails as the example is not matching to the defined type string while it is expecting object

@bganapa , thanks for sharing the detailed information, we will investigate this issue.

@nickzhums
Copy link
Contributor

@lirenhe I posted the question on Teams Channel....
Is there a precedence on how to specify JToken parameter in swagger object model?We have specified the type as string in for the JToken param. The Oav validate-example fails as the example is not matching to the defined type string while it is expecting object

@lirenhe I posted the question on Teams Channel....
Is there a precedence on how to specify JToken parameter in swagger object model?We have specified the type as string in for the JToken param. The Oav validate-example fails as the example is not matching to the defined type string while it is expecting object

@bganapa I took a look at your issue, if you are asking how to specify the "type" of a JToken parameter, you can try using "object" instead of "string". Please refer to the following link:
Azure/oav#78
I modified your JToken parameters locally from "string" to "object" and it passed the validation. Let us know if it works and please comment if you have further questions.

@AutorestCI
Copy link

AutorestCI commented Oct 29, 2019

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@viananth
Copy link
Member Author

@lirenhe I have this avocado failure, where as it fails to resolve $(this-folder) for one json but resolves the same for other json files. I can't repro this on my local dev machine. Seems like an issue in the build. Could you pls take a look?

error: 

code: NO_JSON_FILE_FOUND

message: The JSON file is not found but it is referenced from the readme file.

readMeUrl: >-

  /home/vsts/work/1/c93b354fd9c14905bb574a8834c4d69b/specification/azsadmin/resource-manager/deployment/readme.md

level: Error

jsonUrl: >-

  /home/vsts/work/1/c93b354fd9c14905bb574a8834c4d69b/specification/azsadmin/resource-manager/deployment/$(this-folder)/Microsoft.Deployment.Admin/preview/2019-01-01/ProductSecret.json

@viananth
Copy link
Member Author

I fixed the Model validation errors and it doesn't repro on my dev machine. Not sure why CI is still complaining about it:

 error : 

operationId: FileContainers_Create

scenario: Creates a new product package.

source: response

responseCode: '200'

severity: 0

code: OBJECT_ADDITIONAL_PROPERTIES

details:

  code: OBJECT_ADDITIONAL_PROPERTIES

  params:

    - 'properties '

  message: 'Additional properties not allowed: properties '


  path: ''

  title: '#/definitions/FileContainer'

  description: File container entity.

  position:

    line: 163

    column: 26

  url: >-

    specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/FileContainer.json

  directives: {}

  jsonPosition:

    line: 16

    column: 15

  jsonUrl: >-

    specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/examples/FileContainer/Create.json

  jsonPath: ''

@PhoenixHe-NV
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@lirenhe
Copy link
Member

lirenhe commented Nov 1, 2019

I fixed the Model validation errors and it doesn't repro on my dev machine. Not sure why CI is still complaining about it:
error :

operationId: FileContainers_Create

scenario: Creates a new product package.

source: response

responseCode: '200'

severity: 0

code: OBJECT_ADDITIONAL_PROPERTIES

details:

code: OBJECT_ADDITIONAL_PROPERTIES

params:

- 'properties '

message: 'Additional properties not allowed: properties '

path: ''

title: '#/definitions/FileContainer'

description: File container entity.

position:

line: 163

column: 26

url: >-

specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/FileContainer.json

directives: {}

jsonPosition:

line: 16

column: 15

jsonUrl: >-

specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/examples/FileContainer/Create.json

jsonPath: ''

The model validation failed because you added additional space in the property name and value

@ruowan
Copy link
Member

ruowan commented Nov 1, 2019

For avocado error, currently avocado don't support ${ths-folder} feature. So you can ignore avocado validation errors which contains '${this-folder}' path. I will update avocado to support the feature in next version.

@lirenhe
Copy link
Member

lirenhe commented Nov 4, 2019

It would be good if you could also fix the code style issue reported by 'PrettierCheck'

@viananth
Copy link
Member Author

viananth commented Nov 4, 2019

It would be good if you could also fix the code style issue reported by 'PrettierCheck'

@lirenhe I fixed the prettier check issues, still the CI is reporting the same. I fixed all the examples as well.

D:\github\azure-rest-api-specs>prettier -c ./specification/azsadmin/resource-manager/deployment/Microsoft.Deployment.Admin/preview/2019-01-01/*.*

Checking formatting...
All matched files use Prettier code style!

],
"responses": {
"200": {
"description": "OK",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Prettier Check passed now. It was failing because I had to rebase from master.
@lirenhe Everything looks good now except the known avocado issue. We don't need the "default" value for each response. Could you please merge if it looks good otherwise? Thanks!

viananth and others added 3 commits November 5, 2019 13:12
Fixes from PR build

Fix action type for action plan operation

Add response body for operations list example
Fix model validation errors

Fix model validation errors.

Fix model validation errors
@lirenhe
Copy link
Member

lirenhe commented Nov 6, 2019

@viananth, please wait for ARM team to signoff this PR then I could merge it.

@bganapa
Copy link
Member

bganapa commented Nov 6, 2019

@lirenhe This is for AZURESTACK Admin API surface. This does not get deployed to azure. We have our internal AZURESTACK review process and it has already gone through that process.

These specs never go through ARM Team sign off. You can look at any PRs for changes under https://github.com/Azure/azure-rest-api-specs/tree/master/specification/azsadmin/resource-manager

@lirenhe lirenhe removed the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Nov 6, 2019
@lirenhe
Copy link
Member

lirenhe commented Nov 6, 2019

@lirenhe This is for AZURESTACK Admin API surface. This does not get deployed to azure. We have our internal AZURESTACK review process and it has already gone through that process.
These specs never go through ARM Team sign off. You can look at any PRs for changes under https://github.com/Azure/azure-rest-api-specs/tree/master/specification/azsadmin/resource-manager

I checked your last PR #7591, it still requires ARM sign off. As you have this PR reviewed in your internal process, I will merge it.

@yungezz yungezz merged commit abf6236 into Azure:master Nov 6, 2019
@bganapa
Copy link
Member

bganapa commented Nov 6, 2019

@lirenhe Thanks for pointing out the last PR. I am pinging the PR owner offline to loop us in.

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.

9 participants