First pass at AWS Working Group goals #178
Conversation
Lots of room for improvement, including relating existing PRs to any of these pages. If we can automate the generation of the lists from AWS labels, so much the better.
| * `sqs_queue` | ||
| * `sts_assume_role` | ||
|
|
||
| ## Commands for the above list |
There was a problem hiding this comment.
+1 for documenting these commands.
There was a problem hiding this comment.
At some point we might autogenerate some content, but for now, need some mechanism for keeping them up to date by humans!
| When adding new features, those features should have tests. | ||
|
|
||
| # Github PRs | ||
| There are some PRs with tests: |
There was a problem hiding this comment.
You can search for the test_pull_requests and aws labels, which today returns 32 PRs
https://github.com/ansible/ansible/pulls?utf8=✓&q=is%3Apr%20is%3Aopen%20label%3Aaws%20label%3Atest_pull_requests%20
Ignoring new modules takes that down to 23
https://github.com/ansible/ansible/pulls?utf8=✓&q=is%3Apr%20is%3Aopen%20label%3Aaws%20label%3Atest_pull_requests%20-label%3Anew_module%20-label%3Anew_plugin
You could filter further by ignoring feature_pull_requests.
There was a problem hiding this comment.
That label catches PRs that update test/sanity/pep8/legacy-files.txt, which is not really the same as having tests.
There was a problem hiding this comment.
Added to Testing Working Group agenda #114 (comment)
| @@ -9,12 +9,25 @@ There are currently no regular meetings held. The IRC channel is the | |||
| main and official place to contact the members. For specific issues and | |||
There was a problem hiding this comment.
Need #ansible-aws adding somewhere in this.
There was a problem hiding this comment.
That's already in the Contacts at the bottom, but could make it more explicit here too.
There was a problem hiding this comment.
I'm not in the Openstack Community. I'm in the Ansible AWS Working Group.
If there are templates that come from Ansible Working Groups that are successful, then I'll take those on, otherwise I'm happy to try this approach and see what works or not.
We really don't yet have standards about what should be in this README.md.
| @@ -0,0 +1,19 @@ | |||
| # Best practices | |||
There was a problem hiding this comment.
Maybe add in that the Working Group welcomes work to improve the best practices guidelines.
If any of this checks can be automated AWS and Testing Working groups can do this.
There was a problem hiding this comment.
We welcome the improvements from everyone! But can document that more explicitly too.
There was a problem hiding this comment.
I guess it's more "these are the specific things we care about working towards fixing" and in future "these are the modules that need fixing" and "these are the PRs that will address those". I don't mind being overly documented.
|
@willthames If it is no ready to be merged, the safe way would be to add [WIP] to the title. |
|
@dagwieers there's a difference between "I don't think this is ready to merge" which is what [WIP] is for, and asking for it to be signed off before merge. |
| other people's stuff | ||
| * [Best practices](bestpractices.md) - all modules should meet the Ansible best | ||
| practices, such as python coding standards, documentation. | ||
|
|
There was a problem hiding this comment.
Could we add reviewing PRs to the goals? Ideally there would be a few people looking at anything before it is merged and often pull requests are inadvertently ignored because there aren't enough people reviewing. I think that's detrimental to the contributor and in turn might lead to an unenthusiastic WG.
group-aws/boto3.md
Outdated
| | ec2_vpc_subnet_facts | [25374](https://github.com/ansible/ansible/pull/25374) | | ||
| | rds | [25646](https://github.com/ansible/ansible/pull/25646) | | ||
| | rds_param_group | [25345](https://github.com/ansible/ansible/pull/25345) | | ||
|
|
There was a problem hiding this comment.
A couple more:
ec2_eni_facts: ansible/ansible#22941
s3: ansible/ansible#21529
group-aws/integration.md
Outdated
|
|
||
| # State of the codebase | ||
|
|
||
| Modules with non-trivial tests: |
There was a problem hiding this comment.
How are you defining non-trivial?
There was a problem hiding this comment.
There are some modules that have targets under test/integration/targets but tasks/main.yml is empty.
However, rather than define those as trivial, I should just say 'Existing modules with tests'
group-aws/refactor.md
Outdated
| The current IAM module is a monolith. The intention is to replace it | ||
| with | ||
|
|
||
| * [ ] `iam_user` |
There was a problem hiding this comment.
PR for iam_user is in progress: ansible/ansible#26290
|
Hey @willthames what do you think? Has this had sufficient review? |
|
@gregdek I think it has now for a first pass. There are more improvements to be made perhaps around framing overall goals vs mechanisms for achieving those goals, but that could be a new PR |
| ### boto3 | ||
|
|
||
| ``` | ||
| grep -l 'import boto3' *.py | sed 's/\(.*\)\.py$/* `\1`/' |
There was a problem hiding this comment.
Can we just grep for HAS_BOTO and HAS_BOTO3? I think all modules use those. This wouldn't catch from boto3 import ....
| modules allows us to break backward compatibility which can really | ||
| help with boto3 moves) | ||
|
|
||
| * `ec2` → `ec2_instance` |
There was a problem hiding this comment.
I'd much prefer rewriting a smaller module and calling that ec2_instance since the current ec2 is massive, and this would be an opportunity to clean up all the various options and features that have crept in over time.
|
OK, now waiting on review of @ryansb review. Ping me when you want me to merge. :) |
| * [Move to boto3](boto3.md) - all AWS modules should move to boto3, rather | ||
| than boto. This is happening organically, especially as newer features | ||
| typically exist in boto3 only, but we might want to improve this. | ||
| * [Integration tests](integration.md) - all AWS modules should have integration |
There was a problem hiding this comment.
some of these goals seem to me to be a bit detailed and should probably be moved to a separate "software quality" section or document. Maybe we should put things like.
- build modules that fit with the standard ansible way of doing things and integrate AWS services to that
- make it possible to get complete AWS configuration using Ansible;
- make standard important services configurable through properly integrated standard modules
- cooperate with other cloud module teams to try to encourage compatiblity where that does not compromise flexibility or completeness
- support futureproofing by trying to ensure that official AWS libraries and APIs are used to build modules.
|
|
||
| ## Expected test criteria | ||
|
|
||
| * Resource creation |
There was a problem hiding this comment.
- Handling of incorrect arguments such as lack of region and missing parameters.
- Simple error handling (n.b. Unit testing may be better for reasonable coverage here)
|
I'm going to merge this notwithstanding @mikedlr's comments, they will be insight for a future PR |
Lots of room for improvement, including
relating existing PRs to any of these pages.
If we can automate the generation of the lists
from AWS labels, so much the better.