AKV: Bring changes from two recent PRs to 7.2-preview into 7.3-preview #12322

daviddesberg · 2021-01-07T22:13:17Z

Bringing in custom role definitions as well as a fix to KeyOperationResult that were recently merged only into 7.2-preview and not 7.3-preview.

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Changelog

Please ensure to add changelog with this PR by answering the following questions.

What's the purpose of the update?
- new service onboarding
- new API version
- update existing version for new feature
- update existing version to fix swagger quality issue in s360
- Other, please clarify
When you are targeting to deploy new service/feature to public regions? Please provide date, or month to public if date is not available yet.
When you expect to publish swagger? Please provide date, or month to public if date is not available yet.
If it's an update to existing version, please select SDKs of specific language and CLIs that require refresh after swagger is published.
- SDK of .NET (need service team to ensure code readiness)
- SDK of Python
- SDK of Java
- SDK of Js
- SDK of Go
- PowerShell
- CLI
- Terraform
- No, no need to refresh for updates in this PR

Contribution checklist:

I commit to follow the Breaking Change Policy of “no breaking changes
I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

If any further question about AME onboarding or validation tools, please view the FAQ.

ARM API Review Checklist

Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
- Adding new API(s)
- Adding a new API version
- Adding a new service
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist

If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

Removing API(s) in stable version
Removing properties in stable version
Removing API version(s) in stable version
Updating API in stable version with Breaking Change Validation errors
Updating API(s) in preview over 1 year

Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.

Please follow the link to find more details on PR review process.

openapi-workflow-bot · 2021-01-07T22:13:20Z

Hi, @lusitanian Thanks for your PR. I am workflow bot for review process. Here are some small tips.

Please ensure to do self-check against checklists in first PR comment.

PR assignee is the person auto-assigned and responsible for your current PR reviewing and merging.

For specs comparison cross API versions, Use API Specs Comparison Report Generator

If there is CI failure(s), to fix CI error(s) is mandatory for PR merging; or you need to provide justification in PR comment for explanation. How to fix?

Any feedback about review process or workflow bot, pls contact swagger and tools team. [email protected]

openapi-pipeline-app · 2021-01-07T22:13:21Z

Swagger Validation Report

️❌BreakingChange: 1 Errors, 0 Warnings failed [Detail]

Compared Swaggers (Based on Oad v0.8.5)
- original: preview/7.3-preview/keys.json <---> new: preview/7.3-preview/keys.json
- original: preview/7.3-preview/rbac.json <---> new: preview/7.3-preview/rbac.json

Rule	Message
`❌` 1038 - AddedPath	The new version is adding a path that was not found in the old version. New: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L30:5

️⚠️LintDiff: 1 Warnings warning [Detail]

Linted configuring files (Based on source branch, openapi-validator v1.6.0 , classic-openapi-validator v1.1.5 )
- keyvault/data-plane/readme.md tag:package-preview-7.3-preview
Linted configuring files (Based on target branch, openapi-validator v1.6.0 , classic-openapi-validator v1.1.5 )
- keyvault/data-plane/readme.md tag:package-preview-7.3-preview

Rule	Message
⚠️ R2001 - AvoidNestedProperties	Consider using x-ms-client-flatten to provide a better end user experience New: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L519

️️✔️Avocado succeeded [Detail] [Expand]

Validation passes for Avocado.

️❌ModelValidation: 2 Errors, 0 Warnings failed [Detail]

Rule	Message
`❌` DOUBLE_FORWARD_SLASHES_IN_URL	In operation "RoleDefinitions_Delete", example for parameter "scope": "/" starts with a forward slash and the path template: "/{scope}/providers/Microsoft.Authorization/roleDefinitions/{roleDefinitionName}" contains a forward slash before the parameter starts. This will cause double forward slashes in the request url. Thus making it incorrect. Please rectify the example. Url: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L38
`❌` DOUBLE_FORWARD_SLASHES_IN_URL	In operation "RoleDefinitions_Get", example for parameter "scope": "/" starts with a forward slash and the path template: "/{scope}/providers/Microsoft.Authorization/roleDefinitions/{roleDefinitionName}" contains a forward slash before the parameter starts. This will cause double forward slashes in the request url. Thus making it incorrect. Please rectify the example. Url: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L139

️️✔️SemanticValidation succeeded [Detail] [Expand]

Validation passes for SemanticValidation.

️️✔️[Staging] Cross Version BreakingChange (Base on preview version) succeeded [Detail] [Expand]

There are no breaking changes.

️️✔️[Staging] Cross Version BreakingChange (Base on stable version) succeeded [Detail] [Expand]

There are no breaking changes.

️️✔️CredScan succeeded [Detail] [Expand]

There is no credential detected.

_{Posted by Swagger Pipeline | How to fix these errors?}

openapi-pipeline-app · 2021-01-07T22:13:22Z

Swagger Generation Artifacts

️️✔️ azure-sdk-for-go succeeded [Detail] [Expand]

️✔️Succeeded [Logs]Release - Generate from df3ecf8. SDK Automation 14.0.0

command	sh ./initScript.sh ../../../../../azure-sdk-for-go_tmp/initInput.json ../../../../../azure-sdk-for-go_tmp/initOutput.json
command	go run ./tools/generator/main.go ../../../../../azure-sdk-for-go_tmp/generateInput.json ../../../../../azure-sdk-for-go_tmp/generateOutput.json

️✔️keyvault/v7.1/keyvault [View full logs] [Release SDK Changes]
```
info	[Changelog] No exported changes
```
️✔️keyvault/v7.0/keyvault [View full logs] [Release SDK Changes]
```
info	[Changelog] No exported changes
```
️✔️keyvault/2016-10-01/keyvault [View full logs] [Release SDK Changes]
```
info	[Changelog] No exported changes
```
️✔️keyvault/2015-06-01/keyvault [View full logs] [Release SDK Changes]
```
info	[Changelog] No exported changes
```

️✔️preview/keyvault/v7.2-preview/keyvault [View full logs] [Release SDK Changes]

info	[Changelog] ### New Content
info	[Changelog]
info	[Changelog] - New function `RoleDefinitionsClient.DeletePreparer(context.Context, string, string, string) (*http.Request, error)`
info	[Changelog] - New function `RoleDefinitionsClient.Get(context.Context, string, string, string) (RoleDefinition, error)`
info	[Changelog] - New function `RoleDefinitionsClient.GetPreparer(context.Context, string, string, string) (*http.Request, error)`
info	[Changelog] - New function `RoleDefinitionsClient.CreateOrUpdatePreparer(context.Context, string, string, string, RoleDefinitionCreateParameters) (*http.Request, error)`
info	[Changelog] - New function `RoleDefinitionsClient.CreateOrUpdate(context.Context, string, string, string, RoleDefinitionCreateParameters) (RoleDefinition, error)`
info	[Changelog] - New function `RoleDefinitionsClient.CreateOrUpdateSender(*http.Request) (*http.Response, error)`
info	[Changelog] - New function `RoleDefinitionsClient.DeleteSender(*http.Request) (*http.Response, error)`
info	[Changelog] - New function `RoleDefinitionsClient.CreateOrUpdateResponder(*http.Response) (RoleDefinition, error)`
info	[Changelog] - New function `RoleDefinitionsClient.Delete(context.Context, string, string, string) (RoleDefinition, error)`
info	[Changelog] - New function `RoleDefinitionsClient.GetResponder(*http.Response) (RoleDefinition, error)`
info	[Changelog] - New function `RoleDefinitionsClient.GetSender(*http.Request) (*http.Response, error)`
info	[Changelog] - New function `RoleDefinitionsClient.DeleteResponder(*http.Response) (RoleDefinition, error)`
info	[Changelog] - New struct `RoleDefinitionCreateParameters`
info	[Changelog] - New anonymous field `autorest.Response` in struct `RoleDefinition`
info	[Changelog] - New field `AuthenticationTag` in struct `KeyOperationResult`
info	[Changelog] - New field `AdditionalAuthenticatedData` in struct `KeyOperationResult`
info	[Changelog] - New field `Iv` in struct `KeyOperationResult`
info	[Changelog]
info	[Changelog] Total 0 breaking change(s), 16 additive change(s).
info	[Changelog]

️️✔️[Staging] ApiDocPreview succeeded [Detail] [Expand]

 Please click here to preview with your @microsoft account.

_{Posted by Swagger Pipeline | How to fix these errors?}

openapi-workflow-bot · 2021-01-07T22:31:49Z

Hi @lusitanian, Your PR has some issues. Please fix the CI sequentially by following the order of Avocado, semantic validation, model validation, breaking change, lintDiff.

Task	How to fix	Priority	Support (Microsoft alias)
Avocado	Fix-Avocado	High	ruowan
Semantic validation	Fix-SemanticValidation-Error	High	raychen, jianyxi
Model validation	Fix-ModelValidation-Error	High	raychen,jianyxi
LintDiff	Fix-LintDiff	high	jianyxi, ruoxuan

If you need further help, please feedback via swagger feedback."

openapi-workflow-bot · 2021-01-07T22:32:14Z

NewApiVersionRequired reason:

A service’s API is a contract with customers and is represented by using the api-version query parameter. Changes such as adding an optional property to a request/response or introducing a new operation is a change to the service’s contract and therefore requires a new api-version value. This is critically important for documentation, client libraries, and customer support.

EXAMPLE: if a customer calls a service in the public cloud using api-version=2020-07-27, the new property or operation may exist but if they call the service in a government cloud, air-gapped cloud, or Azure Stack Hub cloud using the same api-version, the property or operation may not exist. Because there is no clear relationship between the service api-version and the new property/operation, customers can’t trust the documentation and Azure customer have difficulty helping customers diagnose issues. In addition, each client library version documents the service version it supports. When an optional property or new operation is added to a service and its Swagger, new client libraries must be produced to expose this functionality to customers. Without updating the api-version, it is unclear to customers which version of a client library supports these new features.

...data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/DeleteRoleDefinition-example.json

...lt/data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/GetRoleDefinition-example.json

...lt/data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/PutRoleDefinition-example.json

specification/keyvault/data-plane/Microsoft.KeyVault/preview/7.3-preview/rbac.json

jhendrixMSFT

Causing parsing error.

...data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/DeleteRoleDefinition-example.json

jhendrixMSFT

Few minor fixes to new examples.

...data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/DeleteRoleDefinition-example.json

...lt/data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/GetRoleDefinition-example.json

jhendrixMSFT · 2021-01-14T16:07:30Z

There are merge conflicts which need to be resolved.

Also can you please fix the model validation and prettier check CI failures.

… AKV

heaths · 2021-01-19T20:14:34Z

@lusitanian the problem is two instances of this:

In operation "RoleDefinitions_Delete", example for parameter "scope": "/" starts with a forward slash and the path template: "/{scope}/providers/Microsoft.Authorization/roleDefinitions/{roleDefinitionName}" contains a forward slash before the parameter starts. This will cause double forward slashes in the request url. Thus making it incorrect. Please rectify the example.
Url: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L38

I get what's it's saying. Looking at examples, there's one of just "keys" and then one like "/", while "{scope}" should be "/" or "/keys", thereby making the path template "{scope}/providers/...". Does that work? How does ARM handle this? We should probably be copying them.

daviddesberg · 2021-01-19T21:32:46Z

@lusitanian the problem is two instances of this:

In operation "RoleDefinitions_Delete", example for parameter "scope": "/" starts with a forward slash and the path template: "/{scope}/providers/Microsoft.Authorization/roleDefinitions/{roleDefinitionName}" contains a forward slash before the parameter starts. This will cause double forward slashes in the request url. Thus making it incorrect. Please rectify the example.
Url: Microsoft.KeyVault/preview/7.3-preview/rbac.json#L38

I get what's it's saying. Looking at examples, there's one of just "keys" and then one like "/", while "{scope}" should be "/" or "/keys", thereby making the path template "{scope}/providers/...". Does that work? How does ARM handle this? We should probably be copying them.

In ARM near as I can tell the root scope ("/") is rarely if ever used in URIs so it's difficult to say. That said, the service is not picky and will accept "//" or "/" for the root scope. I don't think this is a bug.

heaths · 2021-01-19T21:45:37Z

@jhendrixMSFT is it possible we can exempt this error? I know I couldn't with the plethora of MISSING_REQUIRED_PARAMETER errors due to a bug in the validation. Or can it just be ignored since it isn't a problem?

David and I discussed it offline and alternatives don't seem like good UX. It either means taking "/" out of the scope, which is inconsistent with ARM when using scopes like "keys", or accepting an empty string meaning "/", which feels like magic. SDKs could normalize this, but we'd prefer the swagger work for any client whether we generated it (and customized it) or not.

jhendrixMSFT · 2021-01-19T21:56:37Z

I don't know if this type of failure can be suppressed, I will check. However, the error message states "Please rectify the example." which makes me wonder if changing the examples to "/foo" or the like is sufficient?

heaths · 2021-01-19T23:12:50Z

That could work just to satisfy the error. We'll just hit this again when re-recorded, though.

daviddesberg · 2021-01-20T23:44:11Z

@heaths @jhendrixMSFT it'd be fine to pass validation but it'd be a poor example for the role def cases as generally role definitions will only be created at the root scope.

heaths · 2021-01-21T01:27:17Z

Can we force merge this? This validation is wrong in this case - the URL will be fine, as tested by both the service and client teams.

/cc @lmazuel who's been on a few threads/PRs coming in hot from Key Vault and Managed HSM.

jhendrixMSFT · 2021-01-22T17:33:51Z

From the conversation is sounds to me that the DOUBLE_FORWARD_SLASHES_IN_URL check should only be applicable to ARM. I've opened Azure/oav#567 to track getting that fixed (please speak up now if anybody disagrees).

In the meantime I will work on getting this merged.

Azure#12322) * Bring changes from two recent PRs to 7.2-preview into 7.3-preview for AKV * address review feedback * add missing scope strings to examples * hopefully fixing very broken rebase/merge * re-remove re-appearing trailing commas * one more.. * ran prettier-fix

daviddesberg requested review from herveyw-msft and heaths January 7, 2021 22:13

daviddesberg requested review from jlichwa and RandalliLama as code owners January 7, 2021 22:13

openapi-assignment-bot bot assigned jhendrixMSFT Jan 7, 2021

openapi-pipeline-app bot added the CI-FixRequiredOnFailure label Jan 7, 2021

openapi-pipeline-app bot added the NewApiVersionRequired label Jan 7, 2021

openapi-pipeline-app bot removed the CI-FixRequiredOnFailure label Jan 7, 2021

heaths approved these changes Jan 7, 2021

View reviewed changes

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 7, 2021

herveyw-msft approved these changes Jan 8, 2021

View reviewed changes

heaths approved these changes Jan 9, 2021

View reviewed changes

jhendrixMSFT approved these changes Jan 11, 2021

View reviewed changes

jhendrixMSFT requested changes Jan 11, 2021

View reviewed changes

...data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/DeleteRoleDefinition-example.json Outdated Show resolved Hide resolved

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 11, 2021

jhendrixMSFT requested changes Jan 12, 2021

View reviewed changes

...data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/DeleteRoleDefinition-example.json Outdated Show resolved Hide resolved

...lt/data-plane/Microsoft.KeyVault/preview/7.3-preview/examples/GetRoleDefinition-example.json Outdated Show resolved Hide resolved

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 13, 2021

heaths approved these changes Jan 14, 2021

View reviewed changes

daviddesberg added 3 commits January 14, 2021 10:01

Bring changes from two recent PRs to 7.2-preview into 7.3-preview for…

b6f635a

… AKV

address review feedback

fab2203

add missing scope strings to examples

f918074

daviddesberg force-pushed the feature/sync_7.2_into_7.3 branch from e243300 to ba3379e Compare January 14, 2021 18:14

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 14, 2021

re-remove re-appearing trailing commas

1347b9e

openapi-pipeline-app bot added the CI-FixRequiredOnFailure label Jan 15, 2021

one more..

033248b

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 15, 2021

ran prettier-fix

5eb5b84

openapi-pipeline-app bot added CI-FixRequiredOnFailure and removed CI-FixRequiredOnFailure labels Jan 15, 2021

heaths approved these changes Jan 15, 2021

View reviewed changes

jhendrixMSFT approved these changes Jan 19, 2021

View reviewed changes

lmazuel merged commit df3ecf8 into Azure:master Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AKV: Bring changes from two recent PRs to 7.2-preview into 7.3-preview #12322

AKV: Bring changes from two recent PRs to 7.2-preview into 7.3-preview #12322

daviddesberg commented Jan 7, 2021

openapi-workflow-bot bot commented Jan 7, 2021

openapi-pipeline-app bot commented Jan 7, 2021 •

edited

Loading

openapi-pipeline-app bot commented Jan 7, 2021 •

edited

Loading

openapi-workflow-bot bot commented Jan 7, 2021

openapi-workflow-bot bot commented Jan 7, 2021

jhendrixMSFT left a comment

jhendrixMSFT left a comment

jhendrixMSFT commented Jan 14, 2021

heaths commented Jan 19, 2021

daviddesberg commented Jan 19, 2021

heaths commented Jan 19, 2021

jhendrixMSFT commented Jan 19, 2021

heaths commented Jan 19, 2021

daviddesberg commented Jan 20, 2021

heaths commented Jan 21, 2021

jhendrixMSFT commented Jan 22, 2021

AKV: Bring changes from two recent PRs to 7.2-preview into 7.3-preview #12322

AKV: Bring changes from two recent PRs to 7.2-preview into 7.3-preview #12322

Conversation

daviddesberg commented Jan 7, 2021

Changelog

Contribution checklist:

ARM API Review Checklist

Breaking Change Review Checklist

openapi-workflow-bot bot commented Jan 7, 2021

openapi-pipeline-app bot commented Jan 7, 2021 • edited Loading

Swagger Validation Report

openapi-pipeline-app bot commented Jan 7, 2021 • edited Loading

Swagger Generation Artifacts

openapi-workflow-bot bot commented Jan 7, 2021

openapi-workflow-bot bot commented Jan 7, 2021

jhendrixMSFT left a comment

Choose a reason for hiding this comment

jhendrixMSFT left a comment

Choose a reason for hiding this comment

jhendrixMSFT commented Jan 14, 2021

heaths commented Jan 19, 2021

daviddesberg commented Jan 19, 2021

heaths commented Jan 19, 2021

jhendrixMSFT commented Jan 19, 2021

heaths commented Jan 19, 2021

daviddesberg commented Jan 20, 2021

heaths commented Jan 21, 2021

jhendrixMSFT commented Jan 22, 2021

openapi-pipeline-app bot commented Jan 7, 2021 •

edited

Loading

openapi-pipeline-app bot commented Jan 7, 2021 •

edited

Loading