Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this fix the nightly build?. They look equivalent to me in sematic version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh cool! I had forgotten!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I’ll bring this up to Jeff & Jeremy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-opening!