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.

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

Sts client grants #996

Merged
merged 37 commits into from
Jul 24, 2020
Merged

Sts client grants #996

merged 37 commits into from
Jul 24, 2020

Conversation

egetman
Copy link
Contributor

@egetman egetman commented Jun 29, 2020

Hey, guys.
I've played a little with an STS api, and figured out that there is no implementation in java sdk.
So i've made one for "client grants".

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

  1. Could you adapt credentials framework like in minio-py ?
  2. We do not use get<PROPERTY>() for getters instead just <PROPERTY>() and make property immutable.

@egetman
Copy link
Contributor Author

egetman commented Jun 29, 2020

@balamurugana 1. i'm not sure, i'll take a look at wednesday or sunday, ok?

@egetman
Copy link
Contributor Author

egetman commented Jul 1, 2020

hi, @balamurugana
I've added a few code cleanups, not related to this pr (it was unintentional, automatic =( ), hope that it's ok?

@balamurugana
Copy link
Member

@egetman

hi, @balamurugana
I've added a few code cleanups, not related to this pr (it was unintentional, automatic =( ), hope that it's ok?

Can you make those automatic changes into separate PR? Also you could add how it is generated in the description.

@egetman
Copy link
Contributor Author

egetman commented Jul 1, 2020

I mean i made that changes instinctively during minio client update =D (like removing unnecessary boxing etc...)

@balamurugana
Copy link
Member

@egetman Can you add copyright to newly added files before we start review?

@harshavardhana
Copy link
Member

@balamurugana I find only examples with the copyright labels. Should I add copyrights to all other library-related classes/interfaces?

@egetman all of them

api/src/main/java/io/minio/S3Escaper.java Outdated Show resolved Hide resolved
examples/ClientGrants.java Outdated Show resolved Hide resolved
examples/WebIdentity.java Outdated Show resolved Hide resolved
@egetman
Copy link
Contributor Author

egetman commented Jul 2, 2020

@balamurugana @harshavardhana done

Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Initial basic review.

api/src/main/java/io/minio/messages/Credentials.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/messages/Credentials.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/messages/WebIdentityToken.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/MinioClient.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/MinioClient.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/MinioClient.java Outdated Show resolved Hide resolved
@balamurugana
Copy link
Member

@egetman Please make a separate PR for changes not related to credentials in MinioClient.java. Its hard to review many changes together.

api/src/main/java/io/minio/messages/WebIdentityToken.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/messages/ClientGrantsToken.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/messages/Credentials.java Outdated Show resolved Hide resolved
api/src/main/java/io/minio/messages/Credentials.java Outdated Show resolved Hide resolved
/**
* @return a valid (not expired) {@link Credentials} instance for {@link io.minio.MinioClient}.
*/
Credentials fetch();
Copy link
Member

Choose a reason for hiding this comment

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

You could refer minio-py; but it can be brought later if needed.

api/src/main/java/io/minio/MinioClient.java Outdated Show resolved Hide resolved
@egetman
Copy link
Contributor Author

egetman commented Jul 8, 2020

@balamurugana I'm not a fan of such an api (nullable), but you're the boss. I removed cleanup's from MinioClient.

@egetman
Copy link
Contributor Author

egetman commented Jul 15, 2020

@balamurugana Hi! Is there any news about the pr?

balamurugana
balamurugana previously approved these changes Jul 23, 2020
Copy link
Member

@balamurugana balamurugana left a comment

Choose a reason for hiding this comment

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

Our plan to take this PR as is and modify with subsequent PRs to correctness. Please resolve the conflict.

@egetman
Copy link
Contributor Author

egetman commented Jul 23, 2020

@balamurugana done

@balamurugana balamurugana merged commit f1b4011 into minio:master Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants