Updating identity v2 beta versions for identity tests and reverting to GA version v1 for samples #15654

KarishmaGhiya · 2021-06-09T23:50:02Z

The perf tests and manual tests dependency for identity is updated to 2.0.0-beta.4
The samples of packages are pinned to the GA versions of identity being supported by samples. Since samples are being used by end-users, they should be supported by GA versions and not beta version of a dependency.

jeremymeng

LGTM

deyaaeldeen · 2021-06-10T00:04:22Z

sdk/containerregistry/container-registry/samples/v1/javascript/package.json

@@ -25,6 +25,6 @@
  "dependencies": {
    "@azure/container-registry": "next",
    "dotenv": "latest",
-    "@azure/identity": "^1.1.0"
+    "@azure/identity": "2.0.0-beta.3"


I think this will be overwritten next time the publish command is run. I think this change should happen in dev-tool or specified in package.json's sampleConfiguration entry:

"//sampleConfiguration": { // Any dependency version specifications in this setting will be preferred // over the dependencies specified in your ordinary dependencies or // devDependencies. "dependencyOverrides": { // For example, this setting will cause the generated sample packages // to include a dependency on a beta version of identity instead of the // version that your package depends on (currently ^1.1.0) "@azure/identity": "2.0.0-beta.3" } }

cc @witemple-msft

or maybe I am wrong. @witemple-msft does the versions in the samples' package.json come from the package's package.json?

I'd thought the sample's dependencies would be computed from the main package.json, no?

I think you're right, ignore my request then unless Will says otherwise.

@deyaaeldeen I think the "dependencyOverride" setting in the main package's package.json is when we want to specify an exception for the samples - in this case if we want samples to depend on a version of identity other than the package's dependency, then we'll use that setting.

check-enforcer · 2021-06-10T00:22:31Z

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

richardpark-msft · 2021-06-10T00:29:46Z

sdk/eventhub/event-hubs/samples/browserSample/package.json

@@ -29,7 +29,7 @@
  "sideEffects": false,
  "dependencies": {
    "@azure/event-hubs": "^5.0.0",
-    "@azure/identity": "^1.0.2"
+    "@azure/identity": "2.0.0-beta.3"


Is the Identity GA for 2.x happening soon? I'm wondering why we're updating samples if we don't want users to bake a beta into their programs (ie, if they're using this as an example of what they have to reference).

I think that's a good point. @sadasant Why do we want to update samples' identity dependency?

Also in some of the packages, the dependency for the samples is mentioned as a dev-dep in the package. So in making that transition, there might be situation where samples depend on an identity version other than the version that package takes dev dep on. Thoughts? @sadasant

We want to make sure that as many things that use Identity get to use v2 before we release it. v2 will release on September

anything that runs on CI will potentially help us catch bugs before we release 🙏

For what it's worth, the browser sample is not run on CI. I also am hesitant to depend on a beta in a sample.

Is it ok to use beta for node samples, since those run in ci? @chradek

Our tests should be using the v2-beta of Identity to help with getting test coverage. Samples is for the end customers to use and should not use beta versions. Also, if we are getting some test coverage in samples that we are not getting in our tests, then we should take another look at our tests.

I think the only samples that should use the beta would be samples that require said beta? If this sample works against the GA version of identity, I would think it should target the GA version of identity.

Is it ok to use beta for node samples, since those run in ci?

I'll echo @ramya-rao-a and @xirzec that I don't think samples should use betas unless they need functionality that's only in the beta.

These show up in our published documentation for GA packages, so having a user start by using one of our samples that depends on a beta can lead to a poor experience. We haven't been shy about making breaking changes in between beta versions so I'd hate for a customer to end up depending on something still going through churn because they downloaded an official sample.

richardpark-msft · 2021-06-10T00:39:04Z

common/config/rush/pnpm-lock.yaml

@@ -433,6 +433,30 @@ packages:
      node: '>=8.0.0'
    resolution:
      integrity: sha512-eOHstXRBRntoqBLi3bugYBEHpYkm0JiET6y5+P1fz7dqYRFN6hJW8qMJQtYIzIbpXJfRJTJdoiOS5fDQhsez0A==
+  /@azure/identity/[email protected]:


I don't look too often at these files - what is the _debug suffix here at the end of the version string mean?

That looks like a dependency or a transitive dependency - called "debug" with version 4.3.1

sadasant

Clean! ✨

ramya-rao-a · 2021-06-11T01:11:41Z

@KarishmaGhiya Please update the PR title to reflect the changes being made and the PR description with the reasoning

ramya-rao-a · 2021-06-11T05:29:13Z

sdk/servicebus/service-bus/package.json

@@ -93,6 +93,9 @@
    "requiredResources": {
      "Azure Service Bus": "https://docs.microsoft.com/azure/service-bus-messaging"
    },
+    "dependencyOverrides": {


In Container Registry the dependencyOverrides was under requiredResources while here the two are siblings...

Yes they should be siblings. I updated container registry package.json.

Does this just override the rush config? I'm curious why service-bus needs it but not event hubs.

This is for the serviceBus samples though

sadasant

Let’s go 🚀

sadasant

Let’s go 🚀

chradek · 2021-06-11T17:49:29Z

sdk/eventhub/event-hubs/package.json

@@ -106,7 +106,7 @@
  "devDependencies": {
    "@azure/dev-tool": "^1.0.0",
    "@azure/eslint-plugin-azure-sdk": "^3.0.0",
-    "@azure/identity": "^1.1.0",
+    "@azure/identity": "2.0.0-beta.3",


Wait, why does event hubs get beta.3 and everyone else gets beta.4? I feel left behind 😄

Now it should be on par with everyone 🥇

deyaaeldeen · 2021-06-11T17:52:37Z

sdk/eventhub/event-hubs/package.json

@@ -106,7 +106,7 @@
  "devDependencies": {
    "@azure/dev-tool": "^1.0.0",
    "@azure/eslint-plugin-azure-sdk": "^3.0.0",
-    "@azure/identity": "^1.1.0",
+    "@azure/identity": "2.0.0-beta.3",


is this intentionally beta.3 vs beta.4?

No I was in the middle of conversation with Daniel when I made this beta.3, then we decided to go with beta.4. Thanks!

deyaaeldeen · 2021-06-11T17:54:32Z

sdk/servicebus/service-bus/package.json

@@ -93,6 +93,9 @@
    "requiredResources": {
      "Azure Service Bus": "https://docs.microsoft.com/azure/service-bus-messaging"
    },
+    "dependencyOverrides": {
+      "@azure/identity": "^1.1.0"


why not using ^1.3.0?

This is what the service-bus samples originally supported - ^1.1.0. Didn't want to change that, unless package owner wants to. ^1.1.0 means it will support all versions from 1.1.0 to the latest (1.3.0)

https://github.com/Azure/azure-sdk-for-js/pull/15632/files

deyaaeldeen · 2021-06-11T17:55:14Z

sdk/containerregistry/container-registry/package.json

@@ -87,7 +87,7 @@
  "devDependencies": {
    "@azure/dev-tool": "^1.0.0",
    "@azure/eslint-plugin-azure-sdk": "^3.0.0",
-    "@azure/identity": "^1.1.0",
+    "@azure/identity": "2.0.0-beta.3",


same question about beta3 vs beta4.

KarishmaGhiya · 2021-06-11T18:47:31Z

@jeremymeng We'll need to re-record only node tests for container registry. Browser unit tests work fine with just replacement of tenant-id

…o GA version v1 for samples (Azure#15654) Co-authored-by: Jeremy Meng <[email protected]>

KarishmaGhiya requested review from chradek, deyaaeldeen, HarshaNalluru, jeremymeng, mikeharder, praveenkuttappan, richardpark-msft and witemple-msft as code owners June 9, 2021 23:50

ghost added the Event Hubs label Jun 9, 2021

jeremymeng approved these changes Jun 9, 2021

View reviewed changes

deyaaeldeen requested changes Jun 10, 2021

View reviewed changes

deyaaeldeen approved these changes Jun 10, 2021

View reviewed changes

richardpark-msft reviewed Jun 10, 2021

View reviewed changes

sadasant approved these changes Jun 10, 2021

View reviewed changes

samples identity

9e5fd4b

KarishmaGhiya force-pushed the update-samples-identity branch from 4b3f87a to 9e5fd4b Compare June 10, 2021 20:56

KarishmaGhiya added 3 commits June 10, 2021 16:17

update identity version

778d80b

update version

f1140d8

update identity versions for internal tests

09c4904

KarishmaGhiya requested a review from schaabs as a code owner June 10, 2021 23:58

KarishmaGhiya changed the title ~~samples identity~~ Updating versions of identity for identity tests and reverting for samples Jun 11, 2021

format

dc3a500

KarishmaGhiya changed the title ~~Updating versions of identity for identity tests and reverting for samples~~ Updating identity v2 beta versions for identity tests and reverting to GA version v1 for samples Jun 11, 2021

ramya-rao-a reviewed Jun 11, 2021

View reviewed changes

KarishmaGhiya added 2 commits June 11, 2021 09:42

fix the dependecyOverrides

cf76d8f

replace tenant id

ee6f131

sadasant approved these changes Jun 11, 2021

View reviewed changes

chradek reviewed Jun 11, 2021

View reviewed changes

deyaaeldeen reviewed Jun 11, 2021

View reviewed changes

update identity to beta.4

a1bb3d7

jeremymeng added 2 commits June 11, 2021 19:39

Update node recordings

dc267e6

re-gen nodejs recordings

5f75241

KarishmaGhiya merged commit 18b0c92 into Azure:master Jun 11, 2021

sadasant pushed a commit to sadasant/azure-sdk-for-js that referenced this pull request Jun 21, 2021

Updating identity v2 beta versions for identity tests and reverting t…

fc97408

…o GA version v1 for samples (Azure#15654) Co-authored-by: Jeremy Meng <[email protected]>

Updating identity v2 beta versions for identity tests and reverting to GA version v1 for samples #15654

Updating identity v2 beta versions for identity tests and reverting to GA version v1 for samples #15654

Conversation

KarishmaGhiya commented Jun 9, 2021 • edited Loading

jeremymeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

check-enforcer bot commented Jun 10, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xirzec Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

ramya-rao-a commented Jun 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KarishmaGhiya commented Jun 11, 2021 • edited Loading

KarishmaGhiya commented Jun 9, 2021 •

edited

Loading

xirzec Jun 10, 2021 •

edited

Loading

KarishmaGhiya commented Jun 11, 2021 •

edited

Loading