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

Added optional argument 'aws_session_token' to S3Client #2798

Merged

Conversation

Bonsanto
Copy link
Contributor

@Bonsanto Bonsanto commented Oct 2, 2019

Description

Added optional named argument 'aws_session_token' to luigi.contrib.s3.S3Client class. If this argument is provided then during the s3 method usage it will be used.

Motivation and Context

  • Some organizations provide you with aws_access_key_id, aws_secret_access_key and some times they also provide you with the aws_session_token which is mandatory.

Have you tested this? If so, how?

I ran many of my jobs locally and it works perfectly.

@Bonsanto Bonsanto force-pushed the feature/s3-client-aws-session-provided branch from 2e21e02 to f463631 Compare October 2, 2019 22:29
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

The code generally looks good. Are there any tests you can add? Or is this already covered in existing tests?

@Bonsanto
Copy link
Contributor Author

Bonsanto commented Oct 6, 2019

@dlstadther I'm not sure how to test this feature nevertheless I tried my best adding some tests.

@Bonsanto Bonsanto force-pushed the feature/s3-client-aws-session-provided branch 2 times, most recently from 1b26706 to 73f11cd Compare October 6, 2019 15:04
@Bonsanto Bonsanto force-pushed the feature/s3-client-aws-session-provided branch from 73f11cd to 60a5420 Compare October 6, 2019 15:17
@Bonsanto Bonsanto requested a review from dlstadther November 1, 2019 04:45
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

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

So sorry for my neglect. LGTM.

@dlstadther dlstadther merged commit eb2acbc into spotify:master Nov 1, 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.

2 participants