-
Notifications
You must be signed in to change notification settings - Fork 151
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 requester pays support #168
Conversation
d8046d5
to
f094e31
Compare
Linking to #148, since apparently titles don't get the same treatment as comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for your contribution!
Added some minor style comments.
Thanks for the style comments @dannycjones - I committed all of the changes you suggested! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at this feature!
Added some comments about the CLI argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seconding @monthonk's comments, otherwise LGTM.
Sorry it took so long to accept those changes :) When skimming your changes, I didn't realize you were only changing the cli args struct, I figured you intended on changing the |
1c272c8
to
23ded9d
Compare
Otherwise, this should be ready to merge? @monthonk @dannycjones |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. We need to rebase to resolve the conflict with current main branch and it should be ready.
Signed-off-by: Arjun Vikram <arjvik@gmail.com>
Co-authored-by: Daniel Carl Jones <danny@danielcarl.info> Signed-off-by: Arjun Vikram <arjvik@gmail.com>
Co-authored-by: Daniel Carl Jones <danny@danielcarl.info> Signed-off-by: Arjun Vikram <arjvik@gmail.com>
Co-authored-by: Daniel Carl Jones <danny@danielcarl.info> Signed-off-by: Arjun Vikram <arjvik@gmail.com>
Co-authored-by: monthonk <monthonk@amazon.co.uk> Signed-off-by: Arjun Vikram <arjvik@gmail.com>
23ded9d
to
4e3ae24
Compare
Rebase complete! @monthonk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Merged - thanks for the contribution! 🚀 |
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).