-
Notifications
You must be signed in to change notification settings - Fork 26
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
Modified action.py in order to have filters in a more conveinent way #109
base: master
Are you sure you want to change the base?
Conversation
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.
General comments as well:
- Please add unit tests for the new functionality
- Please add a CHANGELOG entry
- Please add documentation about your change to the README
- Please bump the pack version by 0.1.0 because new functionality is added.
@@ -221,6 +221,19 @@ def do_method(self, module_path, cls, action, **kwargs): | |||
method_fqdn = '%s.%s.%s' % (module_path, cls, action) | |||
self.logger.debug('Calling method "%s" with kwargs: %s' % (method_fqdn, str(kwargs))) | |||
|
|||
if 'Filters' in kwargs.keys(): |
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.
Looks like this is the same copy/paste code below. Please make this a function.
if 'Filters' in kwargs.keys(): | ||
boto3_compliant_filters = [] | ||
for specified_filter in kwargs['Filters']: | ||
keyval = specified_filter.split('=') |
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.
Does this mean its not possible to filter on anything with a = in the value now, as it will get split? For example what happens if I can give an AWS resource a tag or value with an = in?
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.
Yeah you're right, I haven't thought of that. It seems that the equal sign is an allowed character in both tag names and values...
Will try to find a solution to that...
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.
suggestion, make filters
an object, then you can just use the key/value pairs in that dict/object
What I do like about this PR is that it makes the Filters-parameter a bit more readable but at the same time it breaks with the official boto3 specs (see: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ec2.html#EC2.Client.describe_instances) As per the docs, Filters is a list of dicts. Both dicts have 2 objects, Name and Values, first is a string, second a list. Is it possible to support both formats? I guess people working with AWS / boto3 would appreciate the consistency. I will do some more tests with the filters tonight as I don't have a st2-setup with access to AWS available right now but maybe it's just a matter of having a few more examples and improve the documentation a bit. |
I spent some more time on this topic and find out two things regarding the behaviour with the current implementation: |
@winem What needs to change to fix your # 2? |
|
Fixes #108
With this fix, it is easier to specify the filters to use with the
st2 run
command.The
Filters
parameter will now have to be specified like this :If you need to have multiple values for a key, you just have to set the same key multiple times.
For example, if I want to describe all the instances that have the
Name
tag set toprod-web
orprod-db
, I will just use the following command :I will stay available if you have questions.
Best regards