[Identity] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055
Conversation
ghost
added
the
| @@ -80,7 +80,7 @@ | |||
| "homepage": "https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/identity/identity/README.md", | |||
| "sideEffects": false, | |||
| "dependencies": { | |||
| "@azure/core-http": "^1.2.4", | |||
| "@azure/core-http": "^2.0.0", | |||
There was a problem hiding this comment.
Do you have to update the version of core-http? Seems like an unnecessary risk for a hotfix unless we have to take it
There was a problem hiding this comment.
I think its because he needs the fix in #15906
There was a problem hiding this comment.
On that note, should this hotfix branch take the update to core-tracing as well?
There was a problem hiding this comment.
Nah, we can do that when core-tracing goes GA.
There was a problem hiding this comment.
I still don't think this is the right thing to do. I worry about surprising someone with a whole host of changes unintentionally especially a major version change in core-http. Instead (and I know it's more painful, so sorry!), I would recommend releasing a hotfix to core-http with just the fix you need, then using that as the core-http version to release the hotfix to identity
There was a problem hiding this comment.
After investigating about the new types, doing some simple tests and discussing this with @chradek , it seems safe for us to upgrade to core-http 2.0.0 on this Identity release.
There was a problem hiding this comment.
The reason I believe it is safe is because
@azure/identityonly re-exportsPipelineOptionsfor@azure/core-httpPipelineOptionsdoesn't include any enums (doesn't appear to include them directly or transitively)- Other than transpiling core-http to ES2017, there weren't any breaking changes to the interfaces.
- We're bumping identity to 1.4.0 because we're dropping node 8 support, which means ES2017 should be safe to use.
@maorleger is there anything in particular that worries you? Any testing we could do to assuage your concerns?
There was a problem hiding this comment.
I'm fine with going to 1.4.0 with this, but was concerned when going to 1.3.1 👍 thanks for verifying the changes are safe
Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com>
| const { span, updatedOptions: newOptions } = createSpan( | ||
| "IdentityClient-refreshAccessToken", | ||
| options | ||
| ); |
There was a problem hiding this comment.
I ran rushx format and it changed a bunch of files. I rather have these formatting changes than not have them. It will make it easier to do further hotfixes if necessary.
| name: "/tenant/oauth2/v2.0/token" | ||
| } | ||
| ] | ||
| children: [] |
There was a problem hiding this comment.
Core-http changes do change the tracing contexts. I am not too worried about this for this hotfix.
There was a problem hiding this comment.
Is that because of core-http or because the core-tracing used ends up being updated?
There was a problem hiding this comment.
I’ve updated Identity to 1.0.0-preview.12 as part of this PR now! Let me know what you think 🌞
sadasant
requested review from
bterlson,
daviwil,
mikeharder,
praveenkuttappan and
schaabs
as code owners
sadasant
changed the title
Includes an update of core-http on Identity to v2. I will update this after core-http is released with the 305 fix.
After this is merged, I will rename the base branch
hotfix/identity_1.3.1tohotfix/identity_1.4.0before triggering the release build.