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: Set rules->group_name's datatype as raw #971

Conversation

GomathiselviS
Copy link
Contributor

Signed-off-by: GomathiselviS gomathiselvi@gmail.com

SUMMARY

rules-> group_name can accept values of type list and str.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

ec2_security_group.py

ADDITIONAL INFORMATION

Signed-off-by: GomathiselviS <gomathiselvi@gmail.com>
@github-actions
Copy link

github-actions bot commented Aug 17, 2022

Docs Build 📝

Thank you for contribution!✨

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

@ansibullbot
Copy link

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 3m 42s
✔️ build-ansible-collection SUCCESS in 5m 06s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 7m 56s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 8m 19s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 8m 13s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 5m 54s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 4m 56s
✔️ ansible-test-splitter SUCCESS in 2m 59s
✔️ integration-amazon.aws-1 SUCCESS in 11m 50s
⚠️ 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-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
ansible-test-changelog FAILURE in 2m 18s

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

Hi @GomathiselviS,

Thanks for taking the time to open this PR.

Given that string and list-of-string are the valid inputs for the option it would be better to define the type='list' and elements='str'. However, looking at the code, I'm not 100% sure it would actually handle a list rather than just a string...

@GomathiselviS
Copy link
Contributor Author

Hi @GomathiselviS,

Thanks for taking the time to open this PR.

Given that string and list-of-string are the valid inputs for the option it would be better to define the type='list' and elements='str'. However, looking at the code, I'm not 100% sure it would actually handle a list rather than just a string...

Hi @tremble , I tested the module with "group_name" as both str and list of strings, and the task succeeded in both cases. I understand that this line handles the conversion. The doc has examples for both the cases, hence I marked the data type as raw. Please let me know your views.

…e.yml

Co-authored-by: Mark Chappell <mchappel@redhat.com>
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 12s
✔️ build-ansible-collection SUCCESS in 4m 44s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 8m 34s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 9m 51s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 7m 57s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 6m 16s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 32s
✔️ ansible-test-splitter SUCCESS in 2m 34s
✔️ integration-amazon.aws-1 SUCCESS in 12m 01s
⚠️ 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-community.aws-1 SUCCESS in 37m 00s
✔️ integration-community.aws-2 SUCCESS in 54m 07s
⚠️ 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
✔️ ansible-test-changelog SUCCESS in 2m 10s

@tremble
Copy link
Contributor

tremble commented Aug 18, 2022

The doc has examples for both the cases, hence I marked the data type as raw. Please let me know your views.

In that case the best bet is to set type: list and elements: str. Ansible cleanly handles the "single string" use case by automatically turning that into a list with 1 item.

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

ansible-galaxy-importer FAILURE in 4m 41s
✔️ build-ansible-collection SUCCESS in 5m 08s
ansible-test-sanity-aws-ansible-python38 FAILURE in 7m 47s
ansible-test-sanity-aws-ansible-2.12-python38 FAILURE in 8m 49s
ansible-test-sanity-aws-ansible-2.13-python38 FAILURE in 7m 45s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 09s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 44s
✔️ ansible-test-splitter SUCCESS in 2m 58s
✔️ integration-amazon.aws-1 SUCCESS in 13m 43s
⚠️ 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-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
✔️ ansible-test-changelog SUCCESS in 2m 15s

@GomathiselviS
Copy link
Contributor Author

recheck

@ansibullbot ansibullbot removed the small_patch Hopefully easy to review label Aug 23, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

✔️ ansible-galaxy-importer SUCCESS in 4m 34s
✔️ build-ansible-collection SUCCESS in 5m 06s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 9m 57s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 10m 50s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 7m 53s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 22s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 6m 38s
✔️ ansible-test-splitter SUCCESS in 2m 57s
✔️ integration-amazon.aws-1 SUCCESS in 13m 51s
⚠️ 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-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
✔️ ansible-test-changelog SUCCESS in 2m 17s

@tremble tremble added backport-3 PR should be backported to the stable-3 branch mergeit Merge the PR (SoftwareFactory) backport-4 PR should be backported to the stable-4 branch labels Aug 23, 2022
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

✔️ ansible-galaxy-importer SUCCESS in 4m 54s
✔️ build-ansible-collection SUCCESS in 5m 04s
✔️ ansible-test-sanity-aws-ansible-python38 SUCCESS in 9m 12s
✔️ ansible-test-sanity-aws-ansible-2.12-python38 SUCCESS in 8m 13s
✔️ ansible-test-sanity-aws-ansible-2.13-python38 SUCCESS in 10m 55s
✔️ ansible-test-units-amazon-aws-python38 SUCCESS in 7m 08s
✔️ ansible-test-units-amazon-aws-python39 SUCCESS in 7m 08s
✔️ ansible-test-splitter SUCCESS in 2m 35s
✔️ integration-amazon.aws-1 SUCCESS in 14m 58s
⚠️ 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-community.aws-1 SUCCESS in 31m 29s
✔️ integration-community.aws-2 SUCCESS in 55m 07s
⚠️ 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
✔️ ansible-test-changelog SUCCESS in 2m 13s

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit 9e7ae6c into ansible-collections:main Aug 23, 2022
@patchback
Copy link

patchback bot commented Aug 23, 2022

Backport to stable-3: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-3/9e7ae6cf978004d6d407571d3d386a965af079fd/pr-971

Backported as #973

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

patchback bot pushed a commit that referenced this pull request Aug 23, 2022
ec2_security_group: Set rules->group_name's datatype as raw

Signed-off-by: GomathiselviS gomathiselvi@gmail.com
SUMMARY
rules-> group_name can accept values of type list and str.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME

ec2_security_group.py
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
(cherry picked from commit 9e7ae6c)
@patchback
Copy link

patchback bot commented Aug 23, 2022

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/9e7ae6cf978004d6d407571d3d386a965af079fd/pr-971

Backported as #974

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

patchback bot pushed a commit that referenced this pull request Aug 23, 2022
ec2_security_group: Set rules->group_name's datatype as raw

Signed-off-by: GomathiselviS gomathiselvi@gmail.com
SUMMARY
rules-> group_name can accept values of type list and str.
ISSUE TYPE

Docs Pull Request

COMPONENT NAME

ec2_security_group.py
ADDITIONAL INFORMATION

Reviewed-by: Mark Chappell <None>
(cherry picked from commit 9e7ae6c)
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Aug 23, 2022
)

[PR #971/9e7ae6cf backport][stable-4] ec2_security_group: Set rules->group_name's datatype as raw

This is a backport of PR #971 as merged into main (9e7ae6c).
Signed-off-by: GomathiselviS gomathiselvi@gmail.com
SUMMARY
rules-> group_name can accept values of type list and str.
ISSUE TYPE


Docs Pull Request

COMPONENT NAME

ec2_security_group.py
ADDITIONAL INFORMATION
softwarefactory-project-zuul bot pushed a commit that referenced this pull request Aug 25, 2022
)

[PR #971/9e7ae6cf backport][stable-3] ec2_security_group: Set rules->group_name's datatype as raw

This is a backport of PR #971 as merged into main (9e7ae6c).
Signed-off-by: GomathiselviS gomathiselvi@gmail.com
SUMMARY
rules-> group_name can accept values of type list and str.
ISSUE TYPE


Docs Pull Request

COMPONENT NAME

ec2_security_group.py
ADDITIONAL INFORMATION

Reviewed-by: Jill R <None>
Reviewed-by: Alina Buzachis <None>
goneri pushed a commit that referenced this pull request Sep 20, 2022
elb_application_lb - treat empty security group as VPC default

SUMMARY

Fixes idempotency issue when security_groups = [] by treating [] as using the VPC's default security group (like it does on creation).
Fixes #28
Used same logic as amazon.aws.ec2_vpc_route_table does for using default igw
Added integration tests

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elb_application_lb

Reviewed-by: Jill R <None>
Reviewed-by: Mark Woolley <mw@marknet15.com>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@20b726a
GomathiselviS pushed a commit to GomathiselviS/amazon.aws that referenced this pull request Sep 20, 2022
…le-collections#971)

elb_application_lb - treat empty security group as VPC default

SUMMARY

Fixes idempotency issue when security_groups = [] by treating [] as using the VPC's default security group (like it does on creation).
Fixes ansible-collections#28
Used same logic as amazon.aws.ec2_vpc_route_table does for using default igw
Added integration tests

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elb_application_lb

Reviewed-by: Jill R <None>
Reviewed-by: Mark Woolley <mw@marknet15.com>

This commit was initially merged in https://github.com/ansible-collections/community.aws
See: ansible-collections/community.aws@20b726a
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…le-collections#971)

elb_application_lb - treat empty security group as VPC default

SUMMARY

Fixes idempotency issue when security_groups = [] by treating [] as using the VPC's default security group (like it does on creation).
Fixes ansible-collections#28
Used same logic as amazon.aws.ec2_vpc_route_table does for using default igw
Added integration tests

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elb_application_lb

Reviewed-by: Jill R <None>
Reviewed-by: Mark Woolley <mw@marknet15.com>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Sep 18, 2023
…le-collections#971)

elb_application_lb - treat empty security group as VPC default

SUMMARY

Fixes idempotency issue when security_groups = [] by treating [] as using the VPC's default security group (like it does on creation).
Fixes ansible-collections#28
Used same logic as amazon.aws.ec2_vpc_route_table does for using default igw
Added integration tests

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elb_application_lb

Reviewed-by: Jill R <None>
Reviewed-by: Mark Woolley <mw@marknet15.com>
abikouo pushed a commit to abikouo/amazon.aws that referenced this pull request Oct 24, 2023
…le-collections#971)

elb_application_lb - treat empty security group as VPC default

SUMMARY

Fixes idempotency issue when security_groups = [] by treating [] as using the VPC's default security group (like it does on creation).
Fixes ansible-collections#28
Used same logic as amazon.aws.ec2_vpc_route_table does for using default igw
Added integration tests

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME
elb_application_lb

Reviewed-by: Jill R <None>
Reviewed-by: Mark Woolley <mw@marknet15.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-3 PR should be backported to the stable-3 branch backport-4 PR should be backported to the stable-4 branch community_review docs mergeit Merge the PR (SoftwareFactory) module module plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants