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.

Sign up for GitHub

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

Jump to bottom

Split AWS Config modules #40111

Merged
merged 18 commits into from
May 24, 2018
Merged

Split AWS Config modules #40111

merged 18 commits into from
May 24, 2018

Conversation

ryansb
Copy link
Contributor

@ryansb ryansb commented May 14, 2018

SUMMARY

Takes the single big module proposed by @slapula #39080 and pulls them into smaller modules for easier docs/usage.

These are all in one PR for now, but I can pull them apart

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

AWS Config

ANSIBLE VERSION
ADDITIONAL INFORMATION

@ansibot
Copy link
Contributor

ansibot commented May 14, 2018

@ryansb this PR contains more than one new module.

Please submit only one new module per pullrequest. For further explanation, please read grouped module documentation

click here for bot help

@ansibot ansibot added aws cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests. labels May 14, 2018
@slapula
Copy link
Contributor

slapula commented May 14, 2018

Thanks @ryansb. As a general rule, is the preferred pattern here to err on the side of keeping modules small and specific vs large and comprehensive? I actually have an AWS Storage Gateway module that's pretty much done but it's very much like my large AWS Config module. If keeping modules small is preferred then I'll go ahead and break that one up too before I submit it.

@ansibot
Copy link
Contributor

ansibot commented May 14, 2018

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/amazon/aws_config_delivery_channel.py:111:17: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/aws_config_delivery_channel.py:136:21: E128 continuation line under-indented for visual indent

click here for bot help

@ryansb
Copy link
Contributor Author

ryansb commented May 15, 2018

@slapula Yeah, generally we prefer to go small and then let the modules grow with the services. We used to do bigger modules (like ec2 or ec2_vpc) but the usability suffered as those services added settings and features. Small modules give us a better way to grow over time, and makes documenting/using them easier. The larger config module was extremely nicely written - pulling it into separate ones turned out not to be too invasive - thanks for contributing it with the integration tests.

@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label May 15, 2018
@ansibot
Copy link
Contributor

ansibot commented May 15, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_config_aggregator.py:76:22: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/cloud/amazon/aws_config_aggregator.py:76:22: error EXAMPLES: syntax error: mapping values are not allowed here

click here for bot help

@ryansb ryansb added the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label May 15, 2018
@ansibot ansibot added the affects_2.6 This issue/PR affects Ansible v2.6 label May 23, 2018
s-hertel
s-hertel approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

test/integration/targets/aws_config/tasks/main.yaml
register: output
ignore_errors: yes

# - assert:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commented out assertions in the always could be removed.

@s-hertel
Copy link
Contributor

bot_status

@ansibot
Copy link
Contributor

ansibot commented May 24, 2018

Components

lib/ansible/modules/cloud/amazon/aws_config_aggregation_authorization.py
support: community
maintainers:

lib/ansible/modules/cloud/amazon/aws_config_aggregator.py
support: community
maintainers:

lib/ansible/modules/cloud/amazon/aws_config_delivery_channel.py
support: community
maintainers:

lib/ansible/modules/cloud/amazon/aws_config_recorder.py
support: community
maintainers:

lib/ansible/modules/cloud/amazon/aws_config_rule.py
support: community
maintainers:

test/integration/targets/aws_config/aliases
support: community
maintainers:

test/integration/targets/aws_config/defaults/main.yaml
support: community
maintainers:

test/integration/targets/aws_config/files/config-trust-policy.json
support: community
maintainers:

test/integration/targets/aws_config/tasks/main.yaml
support: community
maintainers:

test/integration/targets/aws_config/templates/config-s3-policy.json.j2
support: community
maintainers:

Metadata

waiting_on: ryansb
changes_requested_by: null
needs_info: False
needs_revision: True
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
shippable_status: success
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

@s-hertel
Copy link
Contributor

!needs_revision

@s-hertel s-hertel merged commit 046561b into ansible:devel May 24, 2018
@ryansb ryansb deleted the aws-config-split branch May 24, 2018 22:43
@ryansb ryansb mentioned this pull request May 25, 2018
Closed
@mattclay
Copy link
Member

@ryansb Why is the aws_config test marked disabled?

It looks like it probably should be unsupported instead?

gothicx pushed a commit to gothicx/ansible that referenced this pull request Jun 9, 2018
@ryansb @gothicx
Split AWS Config modules (ansible#40111)
bf6aba3 
* Adding module for AWS Config service

* adding integration tests

* Split resource types into their own modules

* Properly use resource_prefix and retry on IAM "eventual consistency"

* Add config aggregator module

* AWS config aggregator integration test fixes

* AWS config recorder module

* Config aggregation auth rule

* Use resource_prefix in IAM role name

* Disable config tests
jacum pushed a commit to jacum/ansible that referenced this pull request Jun 26, 2018
@ryansb
Split AWS Config modules (ansible#40111)
3eae2fe 
* Adding module for AWS Config service

* adding integration tests

* Split resource types into their own modules

* Properly use resource_prefix and retry on IAM "eventual consistency"

* Add config aggregator module

* AWS config aggregator integration test fixes

* AWS config recorder module

* Config aggregation auth rule

* Use resource_prefix in IAM role name

* Disable config tests
@s-hertel s-hertel removed the needs_ci_update This PR is blocked as it requires an update to CI infrastructure before tests can pass in CI. label Jul 9, 2018
@ansible ansible locked and limited conversation to collaborators May 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@s-hertel s-hertel s-hertel approved these changes

Assignees
No one assigned
Labels
affects_2.6 This issue/PR affects Ansible v2.6 aws cloud module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ryansb @ansibot @slapula @s-hertel @mattclay