Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drksnw
Copy link

@drksnw drksnw commented Jun 8, 2020

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 :

Filters=Key1=Value1,Key2=Value2...

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 to prod-web or prod-db, I will just use the following command :

st2 run aws.ec2_describe_instances Filters=tag:Name=prod-web,tag:Name=prod-db account_id=xxxxxxxxxxxx region=my-aws-region

I will stay available if you have questions.

Best regards

Copy link
Contributor

@nmaludy nmaludy left a 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():
Copy link
Contributor

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('=')
Copy link
Contributor

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?

Copy link
Author

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...

Copy link
Contributor

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

@winem
Copy link

winem commented Jun 9, 2020

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.

@winem
Copy link

winem commented Jun 13, 2020

I spent some more time on this topic and find out two things regarding the behaviour with the current implementation:
1st) I could not figure out a single way where the filter works for the described use-cases
2nd) the issue is that the (kw)args are passed as string and not as dict even if the original object is a dict. So the botocore.exceptions.ParamValidationError will appear every time.

@punkrokk
Copy link
Contributor

I spent some more time on this topic and find out two things regarding the behaviour with the current implementation:
1st) I could not figure out a single way where the filter works for the described use-cases
2nd) the issue is that the (kw)args are passed as string and not as dict even if the original object is a dict. So the botocore.exceptions.ParamValidationError will appear every time.

@winem What needs to change to fix your # 2?

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@cwilson21
Copy link

@punkrokk @winem I don't think this is broken (The original) how it is written. I think that it needs more documentation around it and all the fields do. In the case of Filters for ec2_describe_instances it does not accept single quotes in the JSON you have to provide double quotes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Invalid type for parameter Filters[0] when filtering for tags
7 participants