Skip to content

[Key Vault] Adding preview version 7.0#2367

Merged
jhendrixMSFT merged 7 commits intoAzure:masterfrom
schaabs:7.0-preview
Feb 1, 2018
Merged

[Key Vault] Adding preview version 7.0#2367
jhendrixMSFT merged 7 commits intoAzure:masterfrom
schaabs:7.0-preview

Conversation

@schaabs
Copy link

@schaabs schaabs commented Jan 30, 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

  • 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

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-python

1 similar comment
@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-python

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-python

@jhendrixMSFT
Copy link
Member

@schaabs Hey man there are some modeler validation failures here, can you please take a look? I'm going through the linter failures however I don't think most of them apply to dataplane.

@jhendrixMSFT
Copy link
Member

@schaabs Also the README.md file wasn't updated, is that intentional?

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-go:
Azure/azure-sdk-for-go@ff02b7c

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@b690a4b

@schaabs schaabs requested review from RandalliLama and fearthecowboy and removed request for fearthecowboy January 30, 2018 23:10
@schaabs schaabs requested a review from rijethma January 31, 2018 01:14
@jhendrixMSFT
Copy link
Member

@schaabs There are still some model validation errors, see the log.
You can run the model validation tool locally, there are instructions for installing it here. Then you run oav validate-example <spec-path>.

"body":{
"id":"https://storage-sdk-test.vault-int.azure-int.net/storage/getsas01/sas/getStrgSasDef01",
"sid":"https://storage-sdk-test.vault-int.azure-int.net/secrets/getsas01-getStrgSasDef01",
"template":"se=2018-02-01T00%3A00Z&spr=https&sv=2017-04-17&sr=b&sig=XXFNfuMCHYrBx0bhemJ7PWn0xGfImMXT6LfbXWvtRUk%3D",
Copy link
Contributor

Choose a reason for hiding this comment

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

there are 2 new parameters: "sasType" and "validityPeriod", in all sas get, delete, update, set responses and set request

"sas-definition-name":"setStrgSasDef01",
"api-version":"2016-10-01",
"parameters":{
"template":"se=2018-02-01T00%3A00Z&spr=https&sv=2017-04-17&sr=b&sig=XXFNfuMCHYrBx0bhemJ7PWn0xGfImMXT6LfbXWvtRUk%3D",
Copy link
Contributor

Choose a reason for hiding this comment

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

sasType and validityPeriod are required parameters

"info": {
"title": "KeyVaultClient",
"description": "The key vault client performs cryptographic key operations and vault operations against the Key Vault service.",
"version": "2016-10-01"
Copy link
Contributor

@rijethma rijethma Jan 31, 2018

Choose a reason for hiding this comment

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

we need to change this to reflect v7.0

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I had this change originally but I must have blown it away when I merged in updates from 2016-10-01

Copy link
Contributor

@rijethma rijethma left a comment

Choose a reason for hiding this comment

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

I'll CR once again after you add the missing parameters

"description": "Created certificate bundle.",
"schema": {
"$ref": "#/definitions/CertificateOperation"

Copy link
Contributor

Choose a reason for hiding this comment

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

sasType and validityPeriod are 2 new required parameters here

"description": "Created certificate bundle.",
"schema": {
"$ref": "#/definitions/CertificateOperation"

Copy link
Contributor

Choose a reason for hiding this comment

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

sasType and validityPeriod are 2 new required properties here

@schaabs
Copy link
Author

schaabs commented Feb 1, 2018

@jhendrixMSFT All the model validation failures seem to be saying that path parameters aren't specified, but they are. I believe I've seen this before and was told it was a bug in the validation. Is this the case, or am I incorrectly specifying path parameters in the examples?

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@ffd727f

@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

"description": "Created certificate bundle.",
"schema": {
"$ref": "#/definitions/CertificateOperation"

Copy link
Member

Choose a reason for hiding this comment

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

This is marked as read-only however the property is also required (like 4758). It can't be both, which one is correct?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure what you're referring to here. Is this comment on the wrong line?

Copy link
Member

Choose a reason for hiding this comment

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

Was supposed to be on line 4742, not sure what happened.

],
"paths": {
"/keys/{key-name}/create": {
"post": {
Copy link
Member

Choose a reason for hiding this comment

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

Not all of the operations have examples, will they be added later?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. This version (7.0-preview) of the spec is a copy of the 2016-10-01 spec, with a few breaking changes so far. The 2016-10-01 didn't have examples for these operations. We plan on adding examples for all before moving 7.0 to stable.

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@jhendrixMSFT jhendrixMSFT merged commit bfe1258 into Azure:master Feb 1, 2018
@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

1 similar comment
@AutorestCI
Copy link

This commit was treated and no generation was made for Azure/azure-sdk-for-go

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@c8fe8aa

@AutorestCI
Copy link

Did a commit to Azure/azure-sdk-for-python:
Azure/azure-sdk-for-python@088c1e7

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.

6 participants