Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[identity 2.0] Move identity to using the shared settings. #14986

Merged
merged 5 commits into from
Apr 23, 2021

Conversation

witemple-msft
Copy link
Member

@witemple-msft witemple-msft commented Apr 22, 2021

This is some stuff that I passively cleaned up and modernized while working on the Identity native dependency rework.

  • Moves to the shared rollup config
    • Moves identity/test/assets to identity/assets (same as Form Recognizer)
  • delete karma.conf.js and the unit-test:browser script as we don't support automated browser testing in Identity (IMO leaving it in the repo is confusing and creates the suggestion that automated browser testing should work)

@witemple-msft witemple-msft added Client This issue points to a problem in the data-plane of the library. Azure.Identity labels Apr 22, 2021
@witemple-msft witemple-msft requested a review from sadasant April 22, 2021 19:40
@witemple-msft witemple-msft self-assigned this Apr 22, 2021
@witemple-msft witemple-msft changed the base branch from master to identity-2.0.0-beta.3 April 22, 2021 19:41
@witemple-msft
Copy link
Member Author

My branch to pull native dependencies out of mainline identity is based on this PR, so once It's merged I can rebase that working tree onto the feature branch and put it up for review.

@HarshaNalluru
Copy link
Member

delete karma.conf.js and the unit-test:browser script as we don't support automated browser testing in Identity (IMO leaving it in the repo is confusing and creates the suggestion that automated browser testing should work)

Not just identity, all the packages that depend on identity for testing use the CSCredential. Are you suggesting skipping all of them?

@Azure Azure deleted a comment from check-enforcer bot Apr 22, 2021
@witemple-msft
Copy link
Member Author

@HarshaNalluru No. I should say we do not support automated browser testing of identity. Other packages can continue to use whatever token credential implementation they like for testing, but keep in mind that it's just a hack. What we are testing isn't so much Identity in the browser as much as bearer token authz in the browser, especially since CSC doesn't even work unless we bypass websec. A real user should never use anything other than IBC in a browser, and we are reconsidering even that for v2.

@sadasant
Copy link
Contributor

@willmtemple let's chat! Automated browser testing on Identity v1 and v2 is a thing. It currently works on master. Also, this PR should be made twice, one for the v2 branch (like this one) and another one for master.

@witemple-msft
Copy link
Member Author

@sadasant I can change the base of this PR back to master. Making the same commit twice on two different branches will make merging 2.0.0-beta.3 later challenging, but maybe we can partially merge master back into the feature branch to straighten out the history.

@witemple-msft
Copy link
Member Author

@HarshaNalluru apparently between the time I originally forked this branch off and now, some browser tests for Identity were enabled. So my mistake, but yes we do have some browser tests of Identity now 😄 .

@sadasant
Copy link
Contributor

@willmtemple I'll make sure to keep you up to date! Identity has been moving faster lately 😁

@witemple-msft witemple-msft changed the base branch from identity-2.0.0-beta.3 to master April 23, 2021 19:05
@witemple-msft
Copy link
Member Author

/check-enforcer evaluate

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

Clean and pretty! Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants