Add custom create_tag method to ec2 resource #160
Conversation
| creating multiple tag resources based on the fact you can apply a set | ||
| of tags to multiple ec2 resources. | ||
| """ | ||
| if event_name == 'creating-resource-class.ec2': |
There was a problem hiding this comment.
Is this if statement necessary since this function must always be plugged into the event system using a specific event name?
There was a problem hiding this comment.
I agree, this isn't needed is it?
There was a problem hiding this comment.
Yeah it is needed. The creation of the service resource emits 'creating-resource-class.ec2' while other resources like Instance emits 'creating-resource-class.ec2.Instance'. If I did not have the check, my custom create_tags() method would be added to every resource due to how the event system works.
The only other option would be to add a 'creating-resource-class.ec2.ServiceResource' event when creating the service resource. Thoughts?
|
LGTM |
|
Will the doc system just automatically work with this customization because it's we're replacing an existing method on a resource? Assuming the docs are good, and the unneeded if check for the event is removed, |
|
The docs are fine. The way that the doc system works is that it checks if a method name matches an action name in the resource model. If it does match, it will autogenerate docs for that method and in this case, the custom |
The create tag method cannot be expressed via the resource model because you need to map a set of tag to multiple resources possibly and return a list of each tag for each resource. In order to preserve backward combatability a new custom method needed to be added to the service resource.
Updated create_tags method as well to use the new event name. This also updated the service resource class name to what it is documented as.
|
I updated the event name for the service resource to make it more usable. Also the class name for the service resource was updated along with the |
| class TestCreateTagsInjection(unittest.TestCase): | ||
| def test_create_tags_injected_to_resource(self): | ||
| session = boto3.session.Session(region_name='us-west-2') | ||
| with mock.patch('boto3.ec2.createtags.create_tags') as mock_method: |
There was a problem hiding this comment.
This feels too much like testing implementation details for a functional test. If we move this method or rename it, this test will fail.
If we want something higher level than a unit test, I'd prefer an integration test.
|
The event rename looks good. I would prefer we don't have the functional test or switch it to an integration test. Otherwise looks good. |
|
@jamesls I am leaning toward against adding an integration test for this because in order to reproduce the bug I need to spin up more than one ec2 resources, apply tags, check the tags are there, and then clean them up. It seems that this would be testing the service more than the actual functionality of the customization which is already covered by the unit test. In the end, we just need to make sure the customization overrides the default method. This also raises the question what is the criteria for what goes into the functional test suit? The functional test that was added for all intents and purposes was checking that the resource had the appropriate method, which is what a bunch of the tests in the functional test suite do. The only caveat was that patching was needed to determine if the resource had the appropriate method. Is the criteria that to be in the functional test suite that no mock's or patches are used (unless you are mocking out the make_request call) and is testing the end to end functionality of the feature? Thoughts? |
|
In the meantime, I made the change to switch the functional test to a unit test. |
|
I think the changes look good for now. We can look at improvements in the future. As for functional tests, I think of them as tests that operate at the API used by the customer (and on a technical level don't call out to AWS). The whole idea is to exercise end to end functionality and catch problems such as events not hooked up properly, wrong assumptions about file system access, customizations we make that don't actually work with real world JSON models we use, etc. The more that gets mocked/patched out, the less useful functional tests become for me. You mentioned that are similar functional tests, but I think the differences between them are what make the difference for me. For example, other functional tests only care that FWIW, you could just create a resource that's cheap to create, like But either way, I think it's something we can look at later if this becomes an issue. |
|
Sounds good. Thanks for the clarification. Merging. |
Add custom create_tag method to ec2 resource
The create tag method cannot be expressed via the resource model because you need to map a set of tags to multiple resources possibly and return a list of each tag for each resource. In order to preserve backward compatibility a new custom method needed to be added to the service resource.
Fixes: #137
cc @jamesls @mtdowling