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

Cloudfront sign #1668

Merged
merged 7 commits into from
Dec 8, 2015
Merged

Cloudfront sign #1668

merged 7 commits into from
Dec 8, 2015

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented Dec 4, 2015

This PR depends on boto/botocore#735

This PR also supersedes #1459.

@rayluo rayluo added the pr:needs-review This PR needs a review from a Member. label Dec 4, 2015
@rayluo
Copy link
Contributor Author

rayluo commented Dec 4, 2015

Currently the CI tests fail because the boto/botocore#735 is not yet merged. I run test cases for this PR on my local environment and they pass. Please review. Thanks!

YYYY-MM-DD (which means 0AM UTC of that day),
YYYY-MM-DDThh:mm:ss (with default timezone as UTC),
YYYY-MM-DDThh:mm:ss+hh:mm or YYYY-MM-DDThh:mm:ss-hh:mm (with offset),
or EpochTime.
Copy link
Member

Choose a reason for hiding this comment

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

I assume epoch will also assume UTC? Probably worth documenting.

@jamesls
Copy link
Member

jamesls commented Dec 7, 2015

Just had some small comments.

@jamesls jamesls closed this Dec 7, 2015
@jamesls jamesls reopened this Dec 7, 2015
'required': True,
'help_text': 'The ID of your CloudFront key pairs.',
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the private-key represent? Its good that you put how to specify it but it tells the user nothing about the private key. For a few of these arguments it seems that we are making the assumption the user has read the cloudfront docs. I do not think this should be necessary to use the sign command.

@kyleknap
Copy link
Contributor

kyleknap commented Dec 8, 2015

Looks good. Just had some small comments

@kyleknap
Copy link
Contributor

kyleknap commented Dec 8, 2015

LGTM 🚢

@@ -9,6 +9,7 @@ Next Release (TBD)
(`issue 1664 <https://github.com/aws/aws-cli/pull/1664>`__)
* feature:``aws cloudfront create-invalidation``: Add a new --paths option.
(`issue 1662 <https://github.com/aws/aws-cli/pull/1662>`__)
* feature:``aws cloudfront sign``: Add a new command to sign url.
Copy link
Member

Choose a reason for hiding this comment

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

grammar's off here, maybe "...to create a signed url"?

@jamesls
Copy link
Member

jamesls commented Dec 8, 2015

:shipit:

@jamesls jamesls 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 Dec 8, 2015
rayluo added a commit that referenced this pull request Dec 8, 2015
@rayluo rayluo merged commit 6d61a91 into aws:develop Dec 8, 2015
@rayluo rayluo added feature-request A feature should be added or improved. cloudfront labels Dec 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. pr:ready-to-merge This PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants