[Identity] Update core-client to ^1.4.0 #18776
Conversation
Our internal tests will only pass if we point to 1.3.3 The caveat of this PR is that we won’t be able to release Identity before @azure/core-client, but that should be fine according to our plans anyway. Please let me know if I’m wrong. Feedback always appreciated!
ghost
added
the
sdk/identity/identity/package.json
Outdated
| @@ -95,7 +95,7 @@ | |||
| "@azure/core-util": "^1.0.0-beta.1", | |||
| "@azure/core-tracing": "1.0.0-preview.13", | |||
| "@azure/core-auth": "^1.3.0", | |||
| "@azure/core-client": "^1.0.0", | |||
| "@azure/core-client": "^1.3.3", | |||
There was a problem hiding this comment.
If this change is only to satisfy test then can't we add this as dev dependency? I don't think we should change actual dependency if current one works fine for end users. My understanding of this new feature is : If customer uses new core-client then they can use new core-client AP. Identity will continue to work with older version of core-client but they cannot just access new API in core-client.
There was a problem hiding this comment.
would this fix the nightly build?. They look equivalent to me in sematic version.
There was a problem hiding this comment.
To answer Praveen: core-client is used by Identity’s source. I could add it to devDependencies, but that could lead to inconsistent behavior.
To Jeremy: Well, we know that 1.3.3 is the right version, what I don’t know is if the nightly build will try to use the nightly build of 1.3.3 — if it doesn’t use the nightly build of core-client 1.3.3, then this won’t work.
🤔 I’m not sure what alternative we have. We could also release core-cleint 1.3.3, of course
There was a problem hiding this comment.
@praveenkuttappan, We cannot move it to dev dependency, because the Identity package depends on core-client outside of the tests.
@sadasant, It looks like we are testing unreleased features of a core package in another package which in turn does not use that feature in its implementation, but only has tests that use it. Shouldnt the right place for these tests be in the core package instead?
There was a problem hiding this comment.
@ramya-rao-a There are tests in the core package, but Identity has a recorded test that helped us identify that we weren’t sending the “claims” all the way through. Identity is responsible for sending the claims.
Part of the issue is that I assumed this package was going to be released during the last release week we had, and I didn’t follow up to verify or release it myself.
I am asking Jeff and Jeremy if we should release this package now. Otherwise, I can skip this test until that package is released.
There was a problem hiding this comment.
When rush sees that 1.3.3 is not published, it will pick it up from the main branch.
oh cool! I had forgotten!
There was a problem hiding this comment.
(I’ll bring this up to Jeff & Jeremy)
There was a problem hiding this comment.
@ramya-rao-a I don’t think I should re-open this PR since v1.3.3 isn’t needed for Identity to run, only for Identity tests to run. Should I skip the failing test instead?
There was a problem hiding this comment.
You will still need the changes in this PR. We have the min/max test set up that will run the tests in a package against the min & max version of the dependencies based on semver. Not sure if min/max has been set up for Identity though, but assuming that it has been set up, these tests will fail with run against 1.0.0 of core-client
|
Closed on favor of #18780 |
|
Please update the PR title, description and the file being edited to use ^1.4.0 now that #18780 is merged |
|
@ramya-rao-a in a moment, in a moment |
sadasant
changed the title
|
Thank you both, @ramya-rao-a and @praveenkuttappan . Merging! |
Our internal tests will only pass if we point to 1.4.0
The caveat of this PR is that we won’t be able to release Identity before @azure/core-client, but that should be fine according to our plans anyway.
Please let me know if I’m wrong. Feedback always appreciated!