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

refactor: rewrite AssumeRoleProvider in TypeScript #1140

Merged
merged 12 commits into from
May 17, 2023
Merged

refactor: rewrite AssumeRoleProvider in TypeScript #1140

merged 12 commits into from
May 17, 2023

Conversation

trim21
Copy link
Contributor

@trim21 trim21 commented May 5, 2023 •
edited
Loading

No description provided.

@trim21 trim21 changed the title [WIP]: refactor: rewrite AssumeRoleProvider in TypeScript refactor: rewrite AssumeRoleProvider in TypeScript May 12, 2023
@trim21 trim21 marked this pull request as ready for review May 12, 2023 08:35
@trim21 trim21 marked this pull request as draft May 12, 2023 09:52
@trim21 trim21 marked this pull request as ready for review May 12, 2023 09:55
@trim21
Copy link
Contributor Author

trim21 commented May 12, 2023

Sorry, just found a bug

@trim21
Copy link
Contributor Author

trim21 commented May 13, 2023 •
edited
Loading

There is a bug in Client when using a non-static CredentialProvider.

If CredentialProvider (for example, AssumeRoleProvider) throw a error in getCredentials, Client will just ignore it and continue to run s3 command.

This also cause a unhandledRejection in runtime.

I think this promise rejection should be catched and throw to user.

But this may cause a breaking change, what's maintainers opinion on this?

@harshavardhana @prakashsvmx

also @aldy505

@aldy505
Copy link
Contributor

aldy505 commented May 14, 2023

I need to look at the code to see how this bug is. But usually breaking change for changing the behavior of a bug is acceptable.

As I understand it, if the Client ignored the thrown error, users will not get the results they want right?

aldy505
aldy505 reviewed May 14, 2023
View reviewed changes
src/AssumeRoleProvider.ts Outdated Show resolved Hide resolved
src/AssumeRoleProvider.ts Show resolved Hide resolved
src/AssumeRoleProvider.ts Show resolved Hide resolved
src/AssumeRoleProvider.ts Show resolved Hide resolved
src/CredentialProvider.ts Show resolved Hide resolved
@trim21
Copy link
Contributor Author

trim21 commented May 14, 2023

I need to look at the code to see how this bug is. But usually breaking change for changing the behavior of a bug is acceptable.

As I understand it, if the Client ignored the thrown error, users will not get the results they want right?

ref: #1078

prakashsvmx
prakashsvmx previously approved these changes May 15, 2023
View reviewed changes
Copy link
Member

@prakashsvmx prakashsvmx left a comment

Choose a reason for hiding this comment

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

I tested the basic scenarios. Changes look good to me. 👍

trim21
trim21 commented May 15, 2023
View reviewed changes
src/internal/request.ts Outdated Show resolved Hide resolved
@harshavardhana harshavardhana merged commit 6298bde into minio:master May 17, 2023
@harshavardhana
Copy link
Member

@trim21 I have reached out to you via email. Would you mind responding?

@trim21 trim21 deleted the ts/AssumeRoleProvider branch May 17, 2023 12:41
@VaniHaripriya VaniHaripriya mentioned this pull request Jun 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldy505 aldy505 aldy505 left review comments

@prakashsvmx prakashsvmx prakashsvmx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@trim21 @aldy505 @harshavardhana @prakashsvmx