Add Instance.delete_tags() #459
Conversation
JordonPhillips
added
the
pr/needs-review
|
Amazing. |
| from boto3.resources.model import Action | ||
|
|
||
|
|
||
| def inject_delete_tags(event_name, class_attributes, **kwargs): |
There was a problem hiding this comment.
Not sure if it is a good idea yet, but how about you generate a lazy-loaded docstring and attach it to the method something like this: https://github.com/boto/boto3/blob/develop/boto3/resources/factory.py#L464 in the inject_delete_tags.
I like the idea of modeling it as an actual action so that the documenter will pick it up and document it as a normal method, but I am not a big fan of having to add a new event and register two different handlers to complete the feature. It would be cool if the lazy-loaded docstrings can get flushed out in document_custom_method() so that we do not need to add a new event, we get docstrings for free, and we do not have to add two handlers everytime a resource model is incorrect and we have to add a customization to bridge some logic. Let me know what you think or if it is doable?
|
That is annoying that we cannot fix this. I wish the |
|
Code wise, the actual delete_tags method added looks good. I would like to see a functional test added (possibly with the new stubbing stuff?). So pending:
this gets a |
from me.
JordonPhillips
force-pushed
the
delete_tags
branch
from
54f9fe5
to
ff544d6
Compare
JordonPhillips
added
incorporating-feedback
and removed
pr/needs-review
JordonPhillips
force-pushed
the
delete_tags
branch
3 times, most recently
from
712449d
to
cf3308f
Compare
The Tag resource was incorrectly modeled such that it is not a collection on an Instance. As a result, there is no way to batch delete tags from an Instance resource. This adds that functionality in, in a way that won't break on future EC2 resource changes. In the long term there are plans to codify SDK specific customizations while maintaining a canonically correct resource model, but that will sometime in the future as part of a different story. Fixes boto#381
| @@ -42,19 +42,19 @@ def test_document_interface_is_documented(self): | |||
| request_syntax_contents = self.get_request_syntax_document_block( | |||
| method_contents) | |||
| self.assert_contains_lines_in_order([ | |||
| ' response = table.put_item(', | |||
There was a problem hiding this comment.
So the way our assertion / search works here is that it skips through whitespace to find the selected strings. Asserting whitespace is brittle because we may want to tweak formatting. In this case, the custom methods were being put in the wrong section of the generated docs, and moving them to the correct section changed the indentation somewhat.
JordonPhillips
added
pr/needs-review
|
Changes: Added lots of tests, changed the docs to be lazy-loaded (thus removing the extra event), moved custom methods into the Action section as they should be, fixed some old tests |
| self._emitter.emit( | ||
| 'creating-resource-class.%s' % cls_name, | ||
| class_attributes=attrs, base_classes=base_classes, | ||
| service_context=service_context, emitter=self._emitter) |
There was a problem hiding this comment.
I think I am fine with this change. Having the service_context is a quite informative bit of information that I could see used in helping create the action or base class and the emitter would seem useful to add extensibility logic when creating a method/class such that you could emit and register your own events on the creation of class/method.
@jamesls what are your thoughts on this?
There was a problem hiding this comment.
Service context I see makes sense. I think it's odd to include the emitter in an event emitted by the emitter.
Given you need the emitter to register a function, why not just give the emitter to the class/function on registration instead of at event emission time?
|
Looks good. 🚢 I like that the docstring idea worked out. It will be a lot easier to add custom methods that need to be added for compatibility reasons and can be represented in resource model terms. I do want @jamesls opinion on adding extra arguments to the |
This provides a generalized class that can be used in the future to inject more such actions that can be represented by a model, and then refactors the delete tags injection to utilize it.
JordonPhillips
force-pushed
the
delete_tags
branch
from
5a1bccb
to
0e04fff
Compare
|
Abstracted out a |
| self._emitter.emit( | ||
| 'creating-resource-class.%s' % cls_name, | ||
| class_attributes=attrs, base_classes=base_classes, | ||
| service_context=service_context, resource_name=resource_name) |
There was a problem hiding this comment.
Do we need the resource_name? Can't we just add it to the constructor of CustomModeledAction and inject it as an argument to create_delete_tags_action or pull it off the event name?
There was a problem hiding this comment.
We could potentially pull it off the event name. I didn't want to add it to CustomModeledAction since you would essentially be providing that info twice because it is part of the event name.
|
Looks good. Just had those two comments to help cleanup the interface/speed. |
JordonPhillips
force-pushed
the
delete_tags
branch
from
a8b3305
to
1c7fea7
Compare
|
Updates: |
|
Thanks! 🚢 once test pass. |
Add Instance.delete_tags()
The Tag resource was incorrectly modeled such that it is not a collection on an Instance. As a result, there is no way to batch delete tags from an Instance resource. This adds that functionality in, in a way that won't break on future EC2 resource changes.
In the long term there are plans to codify SDK specific customizations while maintaining a canonically correct resource model, but that will sometime in the future as part of a different story.
Fixes #381
cc @kyleknap @jamesls @mtdowling @rayluo