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.

Sign up for GitHub

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

Jump to bottom

[Identity] [Core] Add support for refresh_on parameter #30402

Merged
merged 29 commits into from
Aug 26, 2024
Merged

[Identity] [Core] Add support for refresh_on parameter #30402

merged 29 commits into from
Aug 26, 2024

Conversation

KarishmaGhiya
Copy link
Member

@KarishmaGhiya KarishmaGhiya commented Jul 13, 2024 •
edited
Loading

Packages impacted by this PR

@azure/core-auth
@azure/core-rest-pipeline
@azure/identity

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)

@KarishmaGhiya KarishmaGhiya force-pushed the refresh-on-support branch 2 times, most recently from 4cff55f to fee0f02 Compare July 13, 2024 01:14
@maorleger maorleger mentioned this pull request Jul 16, 2024
Merged
3 tasks
@KarishmaGhiya KarishmaGhiya marked this pull request as ready for review August 6, 2024 23:37
@KarishmaGhiya KarishmaGhiya requested review from bterlson, a team and schaabs as code owners August 6, 2024 23:37
@KarishmaGhiya
Copy link
Member Author

Update core/ts-http-runtime with changes to AccessToken and tokenCycler after this PR is approved!

@azure-sdk
Copy link
Collaborator

azure-sdk commented Aug 7, 2024 •
edited
Loading

API change check

API changes are not detected in this pull request.

@KarishmaGhiya
Copy link
Member Author

Need to update sendTokenRequest and may be add parseExpiration in the identityClient

xirzec
xirzec reviewed Aug 8, 2024
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Changes look good to me, minor nit on naming and agree with Maor's suggestion

sdk/core/core-auth/src/tokenCredential.ts Outdated Show resolved Hide resolved
maorleger
maorleger approved these changes Aug 9, 2024
View reviewed changes
Copy link
Member

@maorleger maorleger left a comment

Choose a reason for hiding this comment

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

LGTM overall - the only things that I wanted to ask for and will trust your judgement on:

  1. Should we make the same changes in ts-http-runtime?
  2. Can we add some unit tests for the refreshOn parsing and tokenCycler logic?

@KarishmaGhiya
Copy link
Member Author

Add a test like https://github.com/maorleger/azure-sdk-for-js/blob/identity-esm-migration/sdk/core/ts-http-runtime/test/bearerTokenAuthenticationPolicy.spec.ts#L76

  • Setup token's refreshAfter to be Date.now + 5000ms
  • Setup token's expiresOn to be Date.now + 5000ms * 60 * 10 (or something large)
  • Create a request, so the first token is fetched and cached
  • Advance timers by 5000ms * 2 (or something)
  • send a request again
  • Ensure the token was refreshed

@KarishmaGhiya KarishmaGhiya mentioned this pull request Aug 13, 2024
Closed
@KarishmaGhiya KarishmaGhiya enabled auto-merge (squash) August 13, 2024 22:04
@maorleger
Copy link
Member

/azp run js - core - ci

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@KarishmaGhiya
Copy link
Member Author

/check-enforcer evaluate

@KarishmaGhiya KarishmaGhiya enabled auto-merge (squash) August 26, 2024 20:47
@KarishmaGhiya KarishmaGhiya merged commit a8a48e1 into Azure:main Aug 26, 2024
4 checks passed
This was referenced Aug 27, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xirzec xirzec xirzec left review comments

@maorleger maorleger maorleger approved these changes

@bterlson bterlson Awaiting requested review from bterlson bterlson is a code owner

@schaabs schaabs Awaiting requested review from schaabs schaabs is a code owner

@christothes christothes Awaiting requested review from christothes christothes is a code owner automatically assigned from Azure/azure-sdk-write-identity

@g2vinay g2vinay Awaiting requested review from g2vinay g2vinay is a code owner automatically assigned from Azure/azure-sdk-write-identity

@chlowell chlowell Awaiting requested review from chlowell chlowell is a code owner automatically assigned from Azure/azure-sdk-write-identity

@mpodwysocki mpodwysocki Awaiting requested review from mpodwysocki mpodwysocki is a code owner automatically assigned from Azure/azure-sdk-write-identity

@xiangyan99 xiangyan99 Awaiting requested review from xiangyan99 xiangyan99 is a code owner automatically assigned from Azure/azure-sdk-write-identity

Assignees
No one assigned
Labels
Azure.Core Azure.Identity
Projects
Azure Identity SDK Improvements
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@KarishmaGhiya @azure-sdk @maorleger @xirzec