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

ec2_security_group - Move validation for rules into the arg_spec #1214

Conversation

tremble
Copy link
Contributor

@tremble tremble commented Oct 27, 2022

SUMMARY

The bulk of the current rule validation can be better handled by arg_spec parameter validation.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

ec2_security_group

ADDITIONAL INFORMATION

@github-actions
Copy link

github-actions bot commented Oct 27, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

@tremble tremble changed the title Move validation for rules into the arg_spec ec2_security_group - Move validation for rules into the arg_spec Oct 27, 2022
@tremble tremble added the backport-5 PR should be backported to the stable-5 branch label Oct 27, 2022
@ansibullbot
Copy link

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request module module owner_pr PR created by owner/maintainer plugins plugin (any type) labels Oct 27, 2022
@softwarefactory-project-zuul

This comment was marked as outdated.

@softwarefactory-project-zuul

This comment was marked as resolved.

@tremble tremble force-pushed the arg_spec/ec2_security_group/rules branch from 8b57ac0 to 6e927a2 Compare October 27, 2022 14:08
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 10s
✔️ build-ansible-collection SUCCESS in 5m 37s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 9m 20s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 39s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 02s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 7m 19s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 17s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 12s
✔️ cloud-tox-py3 SUCCESS in 4m 01s
✔️ ansible-test-splitter SUCCESS in 2m 44s
✔️ integration-amazon.aws-1 SUCCESS in 11m 23s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 18s

@@ -1170,6 +1192,7 @@ def get_final_rules(client, module, security_group_rules, specified_rules, purge
rule[source_type] = [rule[source_type]]
format_rule[rule_key] = [{source_type: target} for target in rule[source_type]]
if rule.get('group_id') or rule.get('group_name'):
# XXX bug - doesn't cope with a list of ids/names
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing bug - #1217 want to split proper solution as the diff code is gnarly

@tremble tremble marked this pull request as ready for review October 27, 2022 14:59
@tremble tremble force-pushed the arg_spec/ec2_security_group/rules branch from 6e927a2 to 40037c3 Compare October 28, 2022 08:45
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 04s
✔️ build-ansible-collection SUCCESS in 6m 13s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 11m 11s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 40s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 8m 56s
✔️ ansible-test-sanity-aws-ansible-2.14 SUCCESS in 10m 53s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 29s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 27s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 5m 53s
✔️ cloud-tox-py3 SUCCESS in 3m 09s
✔️ ansible-test-splitter SUCCESS in 3m 21s
✔️ integration-amazon.aws-1 SUCCESS in 13m 03s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 59s

@ansibullbot ansibullbot added needs_maintainer new_plugin New plugin tests tests and removed owner_pr PR created by owner/maintainer labels Oct 28, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 08s
✔️ build-ansible-collection SUCCESS in 5m 33s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 26s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 42s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 10m 16s
✔️ ansible-test-sanity-aws-ansible-2.14 SUCCESS in 9m 11s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 6m 51s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 57s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 53s
✔️ cloud-tox-py3 SUCCESS in 3m 09s
✔️ ansible-test-splitter SUCCESS in 2m 32s
✔️ integration-amazon.aws-1 SUCCESS in 12m 17s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 25s

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 3m 47s
✔️ build-ansible-collection SUCCESS in 5m 56s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 10m 00s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 10m 10s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 19s
✔️ ansible-test-sanity-aws-ansible-2.14 SUCCESS in 8m 46s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 7m 39s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 42s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 03s
✔️ cloud-tox-py3 SUCCESS in 4m 23s
✔️ ansible-test-splitter SUCCESS in 2m 42s
✔️ integration-amazon.aws-1 SUCCESS in 13m 07s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-community.aws-1 SKIPPED
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 19s

@tremble tremble added the mergeit Merge the PR (SoftwareFactory) label Nov 1, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.

@tremble
Copy link
Contributor Author

tremble commented Nov 1, 2022

regate

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 3m 37s
✔️ build-ansible-collection SUCCESS in 5m 11s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 10m 05s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 11m 14s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 9m 13s
✔️ ansible-test-sanity-aws-ansible-2.14 SUCCESS in 8m 45s
✔️ ansible-test-units-amazon-aws-python36 SUCCESS in 8m 33s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 48s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 11s
✔️ cloud-tox-py3 SUCCESS in 4m 24s
✔️ ansible-test-splitter SUCCESS in 3m 19s
✔️ integration-amazon.aws-1 SUCCESS in 11m 57s
⚠️ integration-amazon.aws-2 SKIPPED
⚠️ integration-amazon.aws-3 SKIPPED
⚠️ integration-amazon.aws-4 SKIPPED
⚠️ integration-amazon.aws-5 SKIPPED
⚠️ integration-amazon.aws-6 SKIPPED
⚠️ integration-amazon.aws-7 SKIPPED
⚠️ integration-amazon.aws-8 SKIPPED
⚠️ integration-amazon.aws-9 SKIPPED
⚠️ integration-amazon.aws-10 SKIPPED
⚠️ integration-amazon.aws-11 SKIPPED
⚠️ integration-amazon.aws-12 SKIPPED
⚠️ integration-amazon.aws-13 SKIPPED
⚠️ integration-amazon.aws-14 SKIPPED
⚠️ integration-amazon.aws-15 SKIPPED
⚠️ integration-amazon.aws-16 SKIPPED
⚠️ integration-amazon.aws-17 SKIPPED
⚠️ integration-amazon.aws-18 SKIPPED
⚠️ integration-amazon.aws-19 SKIPPED
⚠️ integration-amazon.aws-20 SKIPPED
⚠️ integration-amazon.aws-21 SKIPPED
⚠️ integration-amazon.aws-22 SKIPPED
✔️ integration-community.aws-1 SUCCESS in 26m 57s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED
⚠️ integration-community.aws-19 SKIPPED
⚠️ integration-community.aws-20 SKIPPED
⚠️ integration-community.aws-21 SKIPPED
⚠️ integration-community.aws-22 SKIPPED
✔️ ansible-test-changelog SUCCESS in 2m 26s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 896fe7a into ansible-collections:main Nov 1, 2022
@patchback
Copy link

patchback bot commented Nov 1, 2022

Backport to stable-5: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply 896fe7a on top of patchback/backports/stable-5/896fe7aca231860ee3a573b11cb772f86836c4ea/pr-1214

Backporting merged PR #1214 into main

  1. Ensure you have a local repo clone of your fork. Unless you cloned it
    from the upstream, this would be your origin remote.
  2. Make sure you have an upstream repo added as a remote too. In these
    instructions you'll refer to it by the name upstream. If you don't
    have it, here's how you can add it:
    $ git remote add upstream https://github.com/ansible-collections/amazon.aws.git
  3. Ensure you have the latest copy of upstream and prepare a branch
    that will hold the backported code:
    $ git fetch upstream
    $ git checkout -b patchback/backports/stable-5/896fe7aca231860ee3a573b11cb772f86836c4ea/pr-1214 upstream/stable-5
  4. Now, cherry-pick PR ec2_security_group - Move validation for rules into the arg_spec #1214 contents into that branch:
    $ git cherry-pick -x 896fe7aca231860ee3a573b11cb772f86836c4ea
    If it'll yell at you with something like fatal: Commit 896fe7aca231860ee3a573b11cb772f86836c4ea is a merge but no -m option was given., add -m 1 as follows intead:
    $ git cherry-pick -m1 -x 896fe7aca231860ee3a573b11cb772f86836c4ea
  5. At this point, you'll probably encounter some merge conflicts. You must
    resolve them in to preserve the patch from PR ec2_security_group - Move validation for rules into the arg_spec #1214 as close to the
    original as possible.
  6. Push this branch to your fork on GitHub:
    $ git push origin patchback/backports/stable-5/896fe7aca231860ee3a573b11cb772f86836c4ea/pr-1214
  7. Create a PR, ensure that the CI is green. If it's not — update it so that
    the tests and any other checks pass. This is it!
    Now relax and wait for the maintainers to process your pull request
    when they have some cycles to do reviews. Don't worry — they'll tell you if
    any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@tremble tremble removed the backport-5 PR should be backported to the stable-5 branch label Nov 1, 2022
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Nov 2, 2022
ec2_security_group - refacter get_target_from_rule()

SUMMARY
refacter get_target_from_rule to bring down the complexity score
Builds on top of #1214
ISSUE TYPE

Feature Pull Request

COMPONENT NAME
ec2_security_group
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Nov 8, 2022
Fix ec2_metadata_facts integration test

SUMMARY
With #1214 security_group now validates the rules its passed, requiring a "source" be passed (SG/CIDR).  Previously it just dropped the rule on the floor.  This has however broken the ec2_metadata_facts integration test
ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
ec2_metadata_facts
ADDITIONAL INFORMATION

Reviewed-by: Alina Buzachis <None>
@tremble tremble deleted the arg_spec/ec2_security_group/rules branch February 15, 2023 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request has_issue integration tests/integration mergeit Merge the PR (SoftwareFactory) module module needs_maintainer new_plugin New plugin plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants