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

Ignore paramfile for s3 --website-redirect #1137

Merged
merged 2 commits into from
Feb 10, 2015

Conversation

kyleknap
Copy link
Contributor

@kyleknap kyleknap commented Feb 9, 2015

Fixes #1130

Since we switched the s3 commands to the BasicCommand we picked up the paramfile feature (i.e. file://, http://, etc). Thus we need to blacklist --website-redirect for a couple of the s3 commands.

cc @jamesls @danielgtaylor

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.91% when pulling 7176c3a on kyleknap:website-redirect-s3 into 580d527 on aws:develop.

@@ -37,6 +37,10 @@
'cloudformation.update-stack.stack-policy-url',
'cloudformation.set-stack-policy.stack-policy-url',

'custom.cp.website-redirect',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a side note, I do not like that all custom commands have custom as its service in the event that is emitted. It is not good if there are ever custom commands that overlap in the cli namespace even though they may have a different command lineage.

However, I do not think any change can be made till after the command lineage PR is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, let's get the command lineage PR merged so we can update this PR to avoid custom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. This change may be a little larger than expected (a change in the clidriver and custom commands). If you look further down in the list of black listed arguments, the s3api commands that use --website-redirect have the event listed as s3...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction: the change for the data driven commands need to come in arguments.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for all the spamming. Actually a change will need to take place at unpack_argument in argprocess.py: https://github.com/aws/aws-cli/blob/develop/awscli/argprocess.py#L63

The reason being that the service_name/operation_name input limits all commands to the strict two command length structure.

For example, for the custom top layer s3 command, a custom had to be added to fit the input because both service_name and operation_name are required. This is an issue because if I wanted to apply a handler to the event custom.s3.*.some-argument I would not be able to register to all of s3's operation commands because s3's operation commands must emit s3.operation.some-argument or custom.operation.some-argument to fit the inputs to that function.

This is also an issue for commands that have more than two commands like waiters.

I propose to leave the PR as is and make this change in a subsequent PR because it is outside the scope of the current bug fix as I will have to change the clidriver and some handlers as well.

@jamesls Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

My preference is to just put in the long term fix rather than a temporary fix that will be removed once the changes are plumbed through clidriver/handlers.

However, I'm ok with putting this fix in for the time being. Code and test themselves look good. Feel free to merge if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a reminder to fix it in my latest commit.

kyleknap added a commit that referenced this pull request Feb 10, 2015
Ignore paramfile for s3 --website-redirect
@kyleknap kyleknap merged commit bbf4cfa into aws:develop Feb 10, 2015
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.

Unable to set website-redirect with http:// or https:// prefix
3 participants