[Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

KarishmaGhiya · 2022-02-11T19:54:56Z

Packages impacted by this PR

Identity

Issues associated with this PR

Fixes #20429

Describe the problem that is addressed by this PR

Updating version of identity to new beta

Reason why the build fails :

In the CI, the plugin packages identity-vscode and identity-cache-persistence are built first. Adn these import AzurePluginContext directly from the source files of identity package. The semver version dependency of these packages depend on the 2.0.x versions of identity and upgrading the local version of identity package - no longer satisfies the semver dependency for identity on these plugin packages.
The errors about not being able to find core packages are coming from the src files of identity - that is not yet built and is being pulled in from the src files of the plugin packages. If the reference to AzurePluginContext in the plugin packages was made from the identity package instead of directly importing src files of identity, then we probably would not have run into this issue. It thinks that the identity src files are it's own package files and ofcourse we don't have these core packages as dependencies in the plugin packages.

Design Choices for a solution

Move the definition of AzurePluginContext within the plugin packages OR
Export AzurePluginContext publicly from identity package and modify the references OR
Move the definition of AzurePluginContext into an identity-common folder like we have for keyvault and storage.

With the 3rd option above we will have the following folder structure:

identity/
identity-vscode/
identity-cache-persistence/
identity-common/
--- src/
------ plugins/provider.ts
------ tokenCachePersistenceOptions.ts
------ vsCodeCredentialPlugin.ts

check-enforcer · 2022-02-17T02:45: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

sadasant · 2022-02-22T21:30:33Z

@willmtemple @KarishmaGhiya we’re a bit constrained with time during this Milestone. What about simply updating the Identity version on the plugin package.json files to 2.1.0-beta.1? That way we can prioritize releasing the upcoming betas of Identity and afterwards we can come back to this PR. Let me know what you think 🙂

witemple-msft · 2022-02-22T22:50:31Z

@willmtemple @KarishmaGhiya we’re a bit constrained with time during this Milestone. What about simply updating the Identity version on the plugin package.json files to 2.1.0-beta.1? That way we can prioritize releasing the upcoming betas of Identity and afterwards we can come back to this PR. Let me know what you think 🙂

@sadasant I'm fine with that as long as the plugins don't release a stable version that depends on beta identity and we do it knowing that it's improper for a stable package version to depend on a beta package. (Could/should we pre-emptively update the plugin versions to a beta as well? Maybe not.) At the end of the day either solution is good by me.

witemple-msft

Couple of comments. This is looking more complex than I thought it would be.

witemple-msft · 2022-02-22T19:28:15Z

sdk/identity/identity-common/test/msalTestUtils.ts

@@ -0,0 +1,100 @@
+// Copyright (c) Microsoft Corporation.


I don't think we should move tests to identity-common, just sources.

This is the utility file for the tests that the plugin packages depend on. I had to do that to silence the build errors that I was receiving because of the utils for tests and sources from plugins

witemple-msft · 2022-02-22T19:31:14Z

sdk/identity/identity/package.json

@@ -67,6 +67,7 @@
  "files": [
    "dist/",
    "dist-esm/src/",


Doesn't this need to become dist-esm/identity/src?

witemple-msft · 2022-02-22T19:33:52Z

sdk/identity/identity-cache-persistence/package.json

@@ -61,6 +61,7 @@
  "dependencies": {
    "@azure/core-auth": "^1.3.0",
    "@azure/identity": "^2.0.1",
+    "@azure/msal-common": "^4.5.1",


Why is this change required? Are we importing from it anywhere? Does this have to do with msalTestUtils?

witemple-msft · 2022-02-22T19:35:50Z

sdk/identity/identity-common/src/index.ts

+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+export * from "../src";


I don't understand this. This is src/index.ts, and it does export * from "../src", so doesn't this module refer to this same file?

Do we need this file at all?

witemple-msft · 2022-02-22T23:01:49Z

sdk/identity/identity-common/package.json

@@ -0,0 +1,75 @@
+{
+  "name": "@azure/identity-common",


If we add this package.json here, we need to also add it to the rush configuration and set up a full build setup for it. There are pros and cons to that approach. At the end of the day I don't think that's what we want, and we need to make sure these files in identity-common are isolated modules without any external dependencies. That's problematic for the shared MSAL test configuration.

I think the easiest solution for the test configuration is probably just to duplicate it, much as it may pain me.

witemple-msft · 2022-02-22T23:02:20Z

sdk/identity/identity-vscode/package.json

@@ -60,6 +60,7 @@
  "sideEffects": false,
  "dependencies": {
    "@azure/identity": "^2.0.1",
+    "@azure/msal-common": "^4.5.1",


Same comment about the same line in identity-cache-persistence

witemple-msft · 2022-02-22T23:04:10Z

sdk/identity/identity-common/README.md

@@ -0,0 +1,39 @@
+# Azure Identity Common client library for JavaScript


NIT: This isn't a "client" library and we should freely break the README template here. Just "Azure Identity Common Plugin Library" or something like that.

I do think it's nice to have the README, even if we ditch the package.json or other associated files.

sadasant · 2022-02-23T04:04:02Z

@witemple-msft I'm fine with that as long as the plugins don't release a stable version that depends on beta identity and we do it knowing that it's improper for a stable package version to depend on a beta package.

I completely agree!

Could/should we pre-emptively update the plugin versions to a beta as well?

Why not? I like that idea 🙂 cc: @KarishmaGhiya

What I want to do is for us to focus back on releasing the beta features and then come back to this PR once we’re synced again with other languages.

ghost · 2022-06-17T10:04:40Z

Hi @KarishmaGhiya. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

ghost · 2022-06-24T11:03:43Z

Hi @KarishmaGhiya. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

KarishmaGhiya · 2022-07-02T02:45:50Z

please leave this open for now

ghost · 2022-09-02T10:05:05Z

Hi @KarishmaGhiya. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

ghost · 2022-09-09T11:04:11Z

Hi @KarishmaGhiya. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

KarishmaGhiya requested review from mikeharder, praveenkuttappan, witemple-msft, deyaaeldeen, sadasant and schaabs as code owners February 11, 2022 19:54

ghost added the Azure.Identity label Feb 11, 2022

KarishmaGhiya mentioned this pull request Feb 11, 2022

[identity] log warning for users trying to access cloud shell using user assigned identity #20226

Merged

2 tasks

KarishmaGhiya force-pushed the update-identity-version-beta branch from e6e9dd9 to 922f909 Compare February 17, 2022 03:02

KarishmaGhiya added 4 commits February 18, 2022 14:23

update identity version to beta

a5b880d

update pnpm lock file

0b7cfc4

separate package contents into a common folder

bfda0eb

update the references

b7b929a

KarishmaGhiya force-pushed the update-identity-version-beta branch from b3be27f to b7b929a Compare February 18, 2022 23:26

KarishmaGhiya added 8 commits February 18, 2022 16:09

update pnpm lock

6a43bb8

update the plugins and imports

3f707ea

Merge branch 'main' into update-identity-version-beta

ca81f59

update imports

5aba61a

update references

dc52379

update the links and move some more files to common

346f799

update package.json

c334ec5

update the refs

481a2d4

witemple-msft reviewed Feb 22, 2022

View reviewed changes

KarishmaGhiya changed the title ~~[Identity] Update identity version beta~~ [Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package Apr 15, 2022

ghost added the no-recent-activity There has been no recent activity on this issue. label Jun 17, 2022

ghost closed this Jun 24, 2022

KarishmaGhiya reopened this Jul 2, 2022

ghost removed the no-recent-activity There has been no recent activity on this issue. label Jul 2, 2022

ghost added the no-recent-activity There has been no recent activity on this issue. label Sep 2, 2022

ghost closed this Sep 9, 2022

KarishmaGhiya mentioned this pull request Jan 12, 2023

[Identity] Create identity common package #24445

Closed

2 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

[Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

KarishmaGhiya commented Feb 11, 2022 •

edited

Loading

check-enforcer bot commented Feb 17, 2022

sadasant commented Feb 22, 2022

witemple-msft commented Feb 22, 2022 •

edited

Loading

witemple-msft left a comment

witemple-msft Feb 22, 2022

KarishmaGhiya Feb 23, 2022

witemple-msft Feb 22, 2022

KarishmaGhiya Feb 23, 2022

witemple-msft Feb 22, 2022

witemple-msft Feb 22, 2022

witemple-msft Feb 22, 2022

witemple-msft Feb 22, 2022

witemple-msft Feb 22, 2022

sadasant commented Feb 23, 2022

ghost commented Jun 17, 2022

ghost commented Jun 24, 2022

KarishmaGhiya commented Jul 2, 2022

ghost commented Sep 2, 2022

ghost commented Sep 9, 2022

		@@ -0,0 +1,39 @@
		# Azure Identity Common client library for JavaScript

[Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

[Identity] Update identity plugin file structure so that versions aren't strictly tied to identity package #20340

Conversation

KarishmaGhiya commented Feb 11, 2022 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

Design Choices for a solution

check-enforcer bot commented Feb 17, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

sadasant commented Feb 22, 2022

witemple-msft commented Feb 22, 2022 • edited Loading

witemple-msft 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

sadasant commented Feb 23, 2022

ghost commented Jun 17, 2022

ghost commented Jun 24, 2022

KarishmaGhiya commented Jul 2, 2022

ghost commented Sep 2, 2022

ghost commented Sep 9, 2022

KarishmaGhiya commented Feb 11, 2022 •

edited

Loading

witemple-msft commented Feb 22, 2022 •

edited

Loading