create aws_service_limit module #2775
Conversation
|
Validation tests failed! |
|
Validation tests failed! |
| ta_conn = boto3_conn(module, | ||
| conn_type='client', | ||
| resource='support', | ||
| region=region, |
There was a problem hiding this comment.
i can only get a boto 'support' connection through the region us-east-1. have you tried this on non-us-east-1 regions?
There was a problem hiding this comment.
I haven't tried other regions, no. I'll investigate a bit more.
There was a problem hiding this comment.
Found the answer. There's only one support endpoint. (See: https://docs.aws.amazon.com/general/latest/gr/rande.html#awssupport_region )
I'll fix this.
| result = {} | ||
|
|
||
| # Including ec2 argument spec | ||
| module = AnsibleModule(argument_spec=ec2_argument_spec(), |
There was a problem hiding this comment.
since this tool is really for querying info, would it make sense to allow filtering of the returned data as arguments to the module instead of just dumping everything back to the caller?
There was a problem hiding this comment.
If you look at the test.yml, you'll see how to filter the output from inside a playbook. It's not pretty, but it's doable.
I think supporting filtering in the module itself might be a good feature to add in the next version of this.
| result['aws_service_limits'] = service_limits(ta_conn) | ||
|
|
||
| # Send exit | ||
| module.exit_json(msg="Retrieved ec2 account attributes.", ansible_facts=result) |
There was a problem hiding this comment.
does returning through 'ansible_facts' mean the returned results will be dumped into Ansible's facts for the specific host? if so, should we be associating this non-host-specific data with a host?
our previous attempts at returning data in our modules simply exit_json(results=<result_object_here>). you can then just register the results of the task and access it as a play/playbook variable.
There was a problem hiding this comment.
I'm not familiar enough with the differences between facts and returned data to have a strong opinion.
My general preference tends to favor ease of use, which is why I chose facts. Facts are "just there" without needing the added step of registering the results. It's one less thing for the user to forget to do in their playbook.
In other words, there's no scenario where a consumer wouldn't want the results stored in a variable/fact. So, if I just return the results it means there's a requirement to register the results. It's an added step with no benefit that I can see.
|
This depends on how you want this role/module to get called. If you want this to be a role, then what you have is fine. If you want this to be a module, I think the tasks/main is overkill and not needed. First, we are totally OK/used to/accept that we have to include the role in order to use the module. We use this pattern in a LOT of places. I wouldn't let that deter you from role vs a module. What you have in the tasks file is, in my opinion, not needed. You are doing the module option parsing and validating there. I think this should go into python. Make that region a required var in the python file, then you don't need to check it in the task file. I like doing things in python, it's just much easier than trying to cram logic, even the simple logic, into tasks. Yaml sucks and trying to program with. Python doesn't, i think the code should go into python. To summarize, I would do this:
Then, the user can call your role, use your module, and then register the output like a simple task. |
|
When I looked at other code examples, namely "lib_util", I saw the same basic steps:
It's the same 3 steps every place I looked. So, I think we agree there. For something like lib_util where there's multiple modules in a single role, you don't necessarily want all of them all the time. It makes sense to avoid calling the modules in a task file for that kind of role composition. In my case, my role is only ever going to be a single module. That's why I can get away with putting step 2 into a task file inside the role instead of requiring whomever consumes the module to have to code up all three steps in their playbook. Instead, they just do step 1 & 3. So, hopefully that makes sense. It's honestly a small optimization that makes consuming the module a tiny bit easier. You're absolutely right, it's not strictly necessary. It just makes the module easier to use, IMO. If y'all don't see the merits, I'll remove it. The python module also checks for region. So, that's already there. The assert in the task is redundant, but I put it there to ensure the whole module is well-behaved. I'll add the 'lib_' to the name. FWIW, I looked in the openshift-tools docs and there's no mention of this naming convention. It would be nice if somebody could add some documentation around it. I still don't feel like I have a good understanding of when to use 'lib_' and when not to. Having it written down would help me a lot. Would you mind adding that to the best practices or style guide doc? |
|
I will state my preference. I would rather have the module to do the logic, than call a role that's sole job is to call the module. By using the task file in there, it's basically one way to call the module, and that's all. If the module does need to grow (like adding the filter as suggested by @joelddiaz), then it would be more useful to be able to call the module with options I intend to use. I still hold that it's easier to do the code in python rather than yaml. if you have a task/main.yml, then don't call it a lib_ module. If it doesn't have a task/main, then it should be called a lib_. The lib_ is supposed to mean i can load this, but no tasks are going to run. Hope this makes sense. |
|
That helps. Thanks @mwoodson . I really appreciate your thoughts. 😄 |
|
Validation tests failed! |
|
All tests passed! |
|
All tests passed! |
No description provided.