Use ClientError in EMR customizations #2049
Conversation
When we removed the CLI error handling, we missed this case of using ClientError since it was catching "Exception". This switches to catching ClientError directly.
| list.append({'Role': response['Role'], 'RolePolicy': policy}) | ||
| return list | ||
|
|
||
| def _check_if_role_exists(self, role_name, parsed_globals): |
There was a problem hiding this comment.
Why are we making this public? If it's not intended to be called from other classes I don't think we should be changing internal vs. public for a bug fix.
There was a problem hiding this comment.
The issue is that the command is exceedingly difficult to test. It needs to have some significant refactoring. At the moment though, we have a broken command.
There was a problem hiding this comment.
Just wanted to follow up here. I see the change has been merged and I don't think we need to do anything else here, but I do want to make sure I clarify my thoughts for future changes we may need to make. I realize I wasn't entirely clear on what I meant.
What I was trying to communicate earlier was that we should only make a method public because we want that to be part of the public interface, not because it makes our tests more "correct" because we're not testing internal methods. In general for these types of changes I would prefer to just accept the fact that we're testing internal methods and not make it public. Until such time where we consider rewriting it to be better structured. Consider:
- It's less code (and less risk) than having to find/replace the method name and ensure we change
_footofoo. For these types of changes I think as minimal a change as possible is ideal. - The method really is intended to be internal (i.e if we created another class in this module that was going to use this class, I wouldn't want it calling this internal-but-now-public method).
Yes, avoiding testing internal methods is ideal, but I don't think artificially changing public visibility so we can check the "not-testing-internal-methods" box seems correct.
Hope that helps clarify things going forward.
jamesls
added
incorporating-feedback
and removed
pr:needs-review
JordonPhillips
added
pr:needs-review
|
🚢 I would rather see this be exposed as a public method. It was bad that an internal method was being tested in the first place. |
3 participants
When we removed the CLI error handling, we missed this case of
using ClientError since it was catching "Exception". This switches
to catching ClientError directly.
In the future, I think this entire command and its tests will need
to be refactored.
cc @kyleknap @jamesls