[KeyVault] - Migrate Key Vault Admin package to Core V2 by maorleger · Pull Request #15881 · Azure/azure-sdk-for-js

maorleger · 2021-06-21T19:37:40Z

What

Migrate @azure/keyvault-admin to core V2
Migrate KeyVaultBackupClient and KeyVaultAccessControlClient to core CAE
Bump our minimum @azure/core-lro version to 1.0.6

Why

This PR proves out two important things: it demonstrates that core continuous access evaluation works for both container
registry (already done) and Key Vault (this PR). It also demonstrates the migration path for Core V2 for Key Vault.

The change to core-lro addresses an issue where core-lro was incorrectly depending on core-http (#15880) That has been fixed on 1.0.6
and allows package owners to migrate to core-rest-pipeline and remove core-http without seeing build breaks.

Resolves #15522
Resolves #14306

check-enforcer · 2021-06-21T19:53:01Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

maorleger · 2021-06-21T19:42:01Z

Because core-client depends on core-rest-pipeline 1.0.3 which still references preview 11, we need to downgrade here as well.

Once we GA core-rest-pipeline 1.1.0 and move all the packages to it we should be able to bump everyone to preview 12 in unison

Going to try addressing this in #15881

maorleger · 2021-06-21T20:03:03Z

Moved these here while Azure/autorest.typescript#1013 is investigated

maorleger · 2021-06-21T20:41:31Z

/check-enforcer evaluate

praveenkuttappan · 2021-06-21T20:49:48Z

test comment

praveenkuttappan · 2021-06-21T20:56:46Z

/azp run js - keyvault - ci

azure-pipelines · 2021-06-21T20:57:04Z

Azure Pipelines successfully started running 1 pipeline(s).

maorleger · 2021-06-21T21:15:41Z

/azp run js - keyvault - ci

azure-pipelines · 2021-06-21T21:15:57Z

Azure Pipelines successfully started running 1 pipeline(s).

maorleger · 2021-06-21T21:56:52Z

/azp run js - keyvault - ci

azure-pipelines · 2021-06-21T21:57:10Z

Azure Pipelines successfully started running 1 pipeline(s).

sadasant · 2021-06-21T23:00:08Z

:o this is pretty!

sadasant · 2021-06-21T23:04:34Z

Oh wow! This is a lot more thought through than what we had. I appreciate it!

sadasant

No stones left unturned! Bravo :)

sadasant · 2021-06-22T11:10:25Z

Maor and I have talked about whether this is a breaking change. I don’t think so, but Let’s get @xirzec ‘s thoughts

There are 3 relatively minor breaking changes:

Optional property mode was removed from retryOptions since it only ever had a single enum value and was never used.

handleRedirects was removed from redirectOptions, as the policy can now be controlled by removing it from the pipeline or setting maxRedirects to 0.

keepAliveOptions were removed. Keep alive can be disabled on a per-request basis with disableKeepAlive.

These were all fairly niche options that tended to be used internally by our clients rather than set by consumers, but since we did expose them, we should consider what versioning implication this poses.

Alongside the change to remove _response, a strict semver interpretation would be to major version the package, but debatably a minor bump could be sufficient. /cc @chradek @ramya-rao-a @jeremymeng - I think we should have broad agreement on what our policy is here.

I do plan to merge this PR, but leaving this conversation open if folks want to chime in. I'll also add it to the list of team meeting topics I have here 👍

Since this package is still a beta I am less worried about it and don't think the discussion is blocking this PR

Can we log an issue to ensure the discussion is continued and we come to a conclusion before this package goes GA?

Sure, opened #15916

jeremymeng

Did you re-generate from swagger? I am wondering why there isn't user agent related changes in generated code as we have seen in https://github.com/Azure/azure-sdk-for-js/pull/15777/files#diff-7a8e7d3e28eff9b33911029639e9b1582d1a692a0e39893e9fc6970916506c02

xirzec

Nice work on this! Very clean upgrade.

xirzec · 2021-06-22T23:26:17Z

Since this package is still a beta I am less worried about it and don't think the discussion is blocking this PR

ghost added the KeyVault label Jun 21, 2021

maorleger commented Jun 21, 2021

View reviewed changes

maorleger marked this pull request as ready for review June 21, 2021 20:04

maorleger requested review from bterlson, chradek, deyaaeldeen, jeremymeng, sadasant, witemple-msft and xirzec as code owners June 21, 2021 20:04

maorleger commented Jun 21, 2021

View reviewed changes

Comment thread sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated

praveenkuttappan closed this Jun 21, 2021

praveenkuttappan reopened this Jun 21, 2021

maorleger mentioned this pull request Jun 21, 2021

[core] Update @azure/core-tracing to use @opentelemetry/api@1.0.0 #15883

Merged

sadasant reviewed Jun 21, 2021

View reviewed changes

Comment thread sdk/keyvault/keyvault-admin/src/challengeAuthenticationCallbacks.ts Outdated

Copy link
Copy Markdown

Contributor

sadasant Jun 21, 2021

Choose a reason for hiding this comment

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

:o this is pretty!

sadasant reviewed Jun 21, 2021

View reviewed changes

sadasant approved these changes Jun 21, 2021

View reviewed changes

maorleger force-pushed the keyvault-admin-core-v2 branch from 36a760d to 4dbb469 Compare June 22, 2021 01:06

sadasant reviewed Jun 22, 2021

View reviewed changes

maorleger mentioned this pull request Jun 22, 2021

Move remainder of Key Vault packages to core-rest-pipeline #15896

Closed

3 tasks

jeremymeng approved these changes Jun 22, 2021

View reviewed changes

Comment thread sdk/keyvault/keyvault-admin/package.json Outdated

Comment thread sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated

Comment thread sdk/keyvault/keyvault-admin/src/accessControlClient.ts Outdated

Comment thread sdk/keyvault/keyvault-admin/src/backupClient.ts Outdated

maorleger added 7 commits June 22, 2021 15:17

move scopes to constants

10fa79b

fix lint errors

5e05262

no need to get recorder involved here

8327656

use hardcoded base url

91e09ff

sort dependencies

f27812f

regenerate with service version capabilities and apply feedback

6f6d147

fix format

d651fd1

maorleger force-pushed the keyvault-admin-core-v2 branch from 4252827 to d651fd1 Compare June 22, 2021 22:20

bump core-lro dependencies to 1.0.6 and above

fcf5176

maorleger requested review from mikeharder and praveenkuttappan as code owners June 22, 2021 22:40

xirzec approved these changes Jun 22, 2021

View reviewed changes

bump everyone to core-lro ^1.0.6

c3a334f

maorleger requested review from a team, 0rland0Wats0n, DominikMe, EmmaZhu, KarishmaGhiya, XiaoningLiu, ankitarorabit, jiacfan, joheredi, ljian3377 and vinjiang as code owners June 22, 2021 23:42

maorleger mentioned this pull request Jun 23, 2021

Decide on versioning policy when switching to core-rest-pipeline #15916

Closed

maorleger merged commit b499bfb into Azure:main Jun 23, 2021

maorleger deleted the keyvault-admin-core-v2 branch June 23, 2021 03:27

JonathanCrd mentioned this pull request Apr 29, 2022

[KeyVault] - Migrate KeyVault Certificates, Keys and Secrets to Core V2 #21702

Merged

3 tasks

Conversation

maorleger commented Jun 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

Uh oh!

check-enforcer Bot commented Jun 21, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maorleger commented Jun 21, 2021

Uh oh!

praveenkuttappan commented Jun 21, 2021

Uh oh!

praveenkuttappan commented Jun 21, 2021

Uh oh!

azure-pipelines Bot commented Jun 21, 2021

Uh oh!

maorleger commented Jun 21, 2021

Uh oh!

azure-pipelines Bot commented Jun 21, 2021

Uh oh!

maorleger commented Jun 21, 2021

Uh oh!

azure-pipelines Bot commented Jun 21, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sadasant left a comment

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!

maorleger Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremymeng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

xirzec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maorleger commented Jun 21, 2021 •

edited

Loading

maorleger Jun 23, 2021 •

edited

Loading