-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055
[Identity] [Hotfix] 1.4.0 increased the IMDS MSI default timeout #16055
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its because he needs the fix in #15906
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.
On that note, should this hotfix branch take the update to core-tracing as well?
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.
Nah, we can do that when core-tracing goes GA.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I believe it is safe is because
@azure/identity
only re-exportsPipelineOptions
for@azure/core-http
PipelineOptions
doesn'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that because of core-http or because the core-tracing used ends up being updated?
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’ve updated Identity to 1.0.0-preview.12 as part of this PR now! Let me know what you think 🌞
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.
Looks good to me!
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.1
tohotfix/identity_1.4.0
before triggering the release build.