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 s3 presign command #2113

Merged
merged 5 commits into from
Aug 19, 2016
Merged

Add s3 presign command #2113

merged 5 commits into from
Aug 19, 2016

Conversation

jamesls
Copy link
Member

@jamesls jamesls commented Aug 18, 2016

This command is paired with get-object, so it can
be used to create presigned URLs you can use to retrieve
objects in S3. There's a couple of enhancements I see in
the future:

  • presign-post
  • presign-put
  • Copying the arg table to support all the params used by
    get-object

I also had to make a few misc commits to the testutils module to fix
a few bugs I encountered while working on tests for this code.

Example usage:

$ aws s3 presign s3://bucket/hello.txt --profile default
https://bucket.s3.amazonaws.com/hello.txt?Signature=abc%3D&Expires=1471536728&AWSAccessKeyId=ABC

cc @kyleknap @JordonPhillips

Closes #462

That way we can still use assert_params_for_cmd
for code that does not make client API calls.
This command is paired with `get-object`, so it can
be used to create presigned URLs you can use to retrieve
objects in S3.  There's a couple of enhancements I see in
the future:

* `presign-post`
* `presign-put`
* Copying the arg table to support all the params used by
  `get-object`
USAGE = "<S3Uri>"
ARG_TABLE = [{'name': 'path',
'positional_arg': True, 'synopsis': USAGE},
{'name': 'expires-in', 'nargs': '?',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is '?' is intentional for ``nargsforexpires-in`? Seems strange that you can provide `--expires-in` without a value especially since `default` is already being set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not intentional, I'll remove.

@kyleknap
Copy link
Contributor

Looks good. Pretty straightforward. I had a couple of small questions.


class TestPresignCommand(BaseAWSCommandParamsTest):

maxDiff = None
Copy link
Member

Choose a reason for hiding this comment

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

BaseAWSCommandParamsTest already sets this, does it not carry through?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've gone ahead and removed this.

@JordonPhillips
Copy link
Member

Tiny questions in addition to Kyle's, but looks good.

@jamesls
Copy link
Member Author

jamesls commented Aug 18, 2016

@kyleknap @JordonPhillips feedback incorporated, let me know if I've missed anything.

@jamesls jamesls added pr:needs-review This PR needs a review from a Member. and removed incorporating-feedback labels Aug 18, 2016
@JordonPhillips
Copy link
Member

--    .-""-.
   ) (     )
  (   )   (
     /     )
    (_    _)                     0_,-.__
      (_  )_                     |_.-._/
       (    )                    |_--..\
        (__)                     |__--_/
     |''   ``\                   |
     |        \                  |      /b.
     |         \  ,,,---===?A`\  |  ,==y'
   ___,,,,,---==""\        |M] \ | ;|\ |>
           _   _   \   ___,|H,,---==""""bno,
    o  O  (_) (_)   \ /          _     AWAW/
                     /         _(+)_  dMM/
      \@_,,,,,,---=="   \      \\|//  MW/
--''''"                         ===  d/
                                    //
                                    ,'__________________________
   \    \    \     \               ,/~~~~~~~~~~~~~~~~~~~~~~~~~~~
                         _____    ,'  ~~~   .-""-.~~~~~~  .-""-.
      .-""-.           ///==---   /`-._ ..-'      -.__..-'
            `-.__..-' =====\\\\\\ V/  .---\.
                      ~~~~~~~~~~~~, _',--/_.\  .-""-.
                            .-""-.___` --  \|         -.__..-

@kyleknap
Copy link
Contributor

Thanks. Looks good. 🚢

@kyleknap kyleknap added pr:ready-to-merge This PR is ready to be merged. and removed pr:needs-review This PR needs a review from a Member. labels Aug 19, 2016
@jamesls jamesls merged commit 3498d63 into aws:develop Aug 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-to-merge This PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support generation of signed URL's for S3 access
4 participants