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

Add AWS session token to credentials #16

Conversation

durbon
Copy link
Contributor

@durbon durbon commented Mar 11, 2019

Congratulation for the project! Super useful for us using S3 like Remote Build Cache.

We have done a little change in order to use the AWS credentials as we are using AWS STS credentials which require the session token.

AWS uses the session token to validate the temporary security credentials. https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_use-resources.html

The PR just add a new check to previous BasicAWSCredentials using instead of BasicSessionCredentials

@durbon durbon force-pushed the feature/MOB-1433-add-session-token-to-credentials branch from f0fc16b to 30ad1db Compare March 11, 2019 17:42
@myniva
Copy link
Owner

myniva commented Mar 11, 2019

Hi @durbon , I'm glad you like the plugin and very happy to have you as another contributor.

As far as I can tell for now, your changes make sense to me. I would kindly ask you to revert the formatting of the classes to the style they were before. Makes it a lot easier for me to do the review :-)

Copy link
Owner

@myniva myniva left a comment

Choose a reason for hiding this comment

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

Please revert the format changes.

durbon added 2 commits March 12, 2019 15:26
…S credentials

add new clausure to use the BasicSessionCredentials when it is present
@durbon durbon force-pushed the feature/MOB-1433-add-session-token-to-credentials branch from 30ad1db to c794bd9 Compare March 12, 2019 16:54
@durbon
Copy link
Contributor Author

durbon commented Mar 12, 2019

@myniva Thanks for considering to add the PR into the project
Sorry, for the automatic format changes, I have different project setting. I have reverted the format change, I now think is much clearer

Any doubts, please let me know

Regards

myniva
myniva previously approved these changes Mar 14, 2019
@myniva
Copy link
Owner

myniva commented Mar 14, 2019

Thanks for updating the files. One last request: Please bump the version in version.properties to 0.9.0. After that, I'll merge this ASAP and v0.9.0 will be released automatically.

@durbon
Copy link
Contributor Author

durbon commented Mar 14, 2019

@myniva Done! Thanks!

@myniva myniva merged commit 60abe7c into myniva:master Mar 15, 2019
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