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

Fix regression from client switch over #1593

Merged
merged 7 commits into from
Oct 29, 2015
Merged

Conversation

kyleknap
Copy link
Contributor

The configservice subscribe command was using an attribute that no longer existed and was relying
on an internal attribute...

cc @jamesls @mtdowling @rayluo @JordonPhillips

@jamesls jamesls added the pr:needs-review This PR needs a review from a Member. label Oct 27, 2015
@jamesls
Copy link
Member

jamesls commented Oct 28, 2015

:shipit:

Do we not have any functional tests for this? It would be nice to have a way to catch if this breaks again, even though .meta is pretty much a support API.

@kyleknap
Copy link
Contributor Author

We do not. I can see if we can add functional tests since we do not have any integration tests and since integration tests would be tricky to add.

@kyleknap
Copy link
Contributor Author

@jamesls
I added some functional tests. For the functional test that checks for an existing bucket and creates it when it does not exist, there is a limitation in BaseAWSCommandParamsTest where you cannot specify more than one status code (sort of similar to being able to provide more than one response via parsed_responses, but there is nothing similiar to that for http_response) which causes the entire subscribe command to fail when you set the status code to 404. However, I am able to capture the desired functionality for checking the existence of a bucket and then attempting to create it.

What do you think? I think this would be nice to have but out of the scope of this PR because this is fixing a regression.

@jamesls
Copy link
Member

jamesls commented Oct 28, 2015

@kyleknap am I correct in understanding that the functional test you've added would not have caught the issue fixed in this PR because we're not creating a bucket?

@kyleknap
Copy link
Contributor Author

It would have in the third test. The error happens before CreateBucket: https://github.com/aws/aws-cli/blob/develop/awscli/customizations/configservice/subscribe.py#L154 and I assert CreateBucket is called.

@jamesls
Copy link
Member

jamesls commented Oct 28, 2015

Oh I see. I didn't notice the third test. In that case, I'm not a huge fan of this because it doesn't really reflect reality. It may also cause additional maintenance on us if we ever support multiple status codes in the base test class.

I'm really just after a test that uses a real client to ensure that we catch any interfaces breaking. What if we just instantiate that S3 helper class with a real client, but mock out the responses?

@kyleknap
Copy link
Contributor Author

Finally all of the leaky patches are fixed. That was rough. Good to look at it one more time.

@jamesls
Copy link
Member

jamesls commented Oct 29, 2015

:shipit:

kyleknap added a commit that referenced this pull request Oct 29, 2015
Fix regression from client switch over
@kyleknap kyleknap merged commit 43d622a into aws:develop Oct 29, 2015
thoward-godaddy pushed a commit to thoward-godaddy/aws-cli that referenced this pull request Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants