-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
CloudTrail customization fix to use built-in session #496
Conversation
…om error handling, which is now handled by the built-in CLI error handler. Fixes an issue where --profile would be ignored during service calls.
# Create the custom command, override its internal method | ||
# so only services are created (and not called) | ||
subscribe = CloudTrailSubscribe(session) | ||
subscribe._call = Mock() |
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.
Is it possible to have a test without using internal methods?
In general, it's preferable to test through a public API.
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.
I should be able to refactor the service creation code into a public method.
Looks like this is failing CI, mind taking a look? https://travis-ci.org/aws/aws-cli/builds/14277015 |
Looks like the problem is that I try to create a real session... I'm looking into how to fix this. |
I would encourage you to check out Also I think we should have at least a test that runs the high level command through CLIDriver, e.g. You might also want to check out the |
…he entire pipeline from start to finish via clidriver.
@jamesls here is another attempt which uses |
# future, but for now we just grab that value out of the real | ||
# os.environ so the patched os.environ has this data and | ||
# the CLI works. | ||
self.environ = { |
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.
This looks like duplicate code from tests/unit/__init__.py
. Was there a reason we can't just use that code?
…nd prevent shadowing self in the mocked method.
Use default session for AWS CloudTrail customizations and remove custom error handling, which is now handled by the built-in CLI error handler. Fixes an issue where --profile would be ignored during service calls.
Unit tests continue to pass.