Add module elb_security_group and tests #182

mmoyle · 2020-08-10T16:05:55Z

SUMMARY

Add module

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

elb_security_group

ansibullbot · 2020-08-19T23:22:56Z

cc @jillr @s-hertel @tremble @wimnat
click here for bot help

tremble

Thanks for taking the time to submit this new module.

Most of the suggestions below are nit-picks. However, there are a couple of things

For net-new modules we generally prefer that they support check_mode
When no change is made changed should return False
Please add a changelog fragment: https://docs.ansible.com/ansible/latest/community/development_process.html#changelogs-how-to

tremble · 2020-09-15T13:30:38Z

plugins/modules/elb_security_group.py

+            module.params.get('security_group_ids')
+        )
+
+        result['changed'] = True


This is always going to return true even if nothing changes

Hi @tremble ,I think a change will be made unless an exception is thrown or the same set of security group ids is supplied. I am not sure but boto3 may even set the same sgs again. The function does not return whether or not no change was made, and may in itself may not be idempotent, of course i can look if needed. I could first inspect what sgs are already attached and compare against them, but I am not sure if this information is worth the cost of an extra api call. Or? What do you advise in this situation?

plugins/modules/elb_security_group.py

tests/integration/targets/elb_security_group/tasks/main.yml

tremble · 2020-09-15T13:41:02Z

tests/integration/targets/elb_security_group/tasks/main.yml

+          - "{{ test_subnets_msg['results'][1]['subnet']['id'] }}"
+        state: present
+      register: alb
+    - name: Attach a security group


Please add tests for things like idempotency

tremble · 2020-09-15T13:41:45Z

plugins/modules/elb_security_group.py

+        original_message=''
+    )
+
+    module = AnsibleAWSModule(argument_spec=argument_spec, supports_check_mode=False)


As a net-new module I'd rather see support for check_mode from day one.

plugins/modules/elb_security_group.py

Co-authored-by: Mark Chappell <mchappel@redhat.com>

randomize vcp cidr

…ity.aws into feature/elb-security-group

Co-authored-by: Mark Chappell <mchappel@redhat.com>

…ity.aws into feature/elb-security-group

ansibullbot · 2020-11-16T18:29:47Z

@mmoyle this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

ansibullbot · 2020-11-16T18:29:49Z

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

changelogs/fragments/182-add-elb-security-groups.yml:0:0: invalid section: minor_change

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

plugins/modules/elb_security_group.py:74:11: undefined-variable: Undefined variable 'camel_dict_to_snake_dict'

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

plugins/modules/elb_security_group.py:74:11: undefined-variable: Undefined variable 'camel_dict_to_snake_dict'

The test ansible-test sanity --test validate-modules [explain] failed with 8 errors:

plugins/modules/elb_security_group.py:0:0: parameter-type-not-in-doc: Argument 'aws_ca_bundle' in argument_spec defines type as 'path' but documentation doesn't define type
plugins/modules/elb_security_group.py:0:0: parameter-type-not-in-doc: Argument 'aws_config' in argument_spec defines type as 'dict' but documentation doesn't define type
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'aws_ca_bundle' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'aws_config' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'aws_endpoint_url' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'aws_profile' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'aws_security_token' is listed in the argument_spec, but not documented in the module documentation
plugins/modules/elb_security_group.py:0:0: undocumented-parameter: Argument 'endpoint_url' is listed in the argument_spec, but not documented in the module documentation

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

changelogs/fragments/182-add-elb-security-groups.yml:0:0: invalid section: minor_change

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

plugins/modules/elb_security_group.py:74:11: undefined-variable: Undefined variable 'camel_dict_to_snake_dict'

click here for bot help

tremble · 2022-07-07T09:03:04Z

@mmoyle,

Thanks for taking the time to open this PR.

This PR appears to have stalled out and has a number of open review points.

Looking at the elb_application_lb module we already support managing the Security Groups on an existing ALB. The existing support also appears to be a little more comprehensive, including supporting using security group names and idempotency. As such I'm going to close out this PR.

Add module elb_security_group and tests

3de741b

ansibullbot added the plugins plugin (any type) label Aug 27, 2020

mmoyle added 2 commits September 15, 2020 15:13

refactor: change libary path

16305a3

refactor: change version

32212a4

tremble requested changes Sep 15, 2020

View reviewed changes

mmoyle and others added 9 commits September 17, 2020 16:34

Update version number

1b8f614

Co-authored-by: Mark Chappell <mchappel@redhat.com>

Update description

94805d8

Co-authored-by: Mark Chappell <mchappel@redhat.com>

Update description

47ff02d

Co-authored-by: Mark Chappell <mchappel@redhat.com>

Update description

72efb64

Co-authored-by: Mark Chappell <mchappel@redhat.com>

add message to exception

9484f67

Co-authored-by: Mark Chappell <mchappel@redhat.com>

add osx to .gitignore

75e9db6

randomize vcp cidr

Merge branch 'feature/elb-security-group' of github.com:mmoyle/commun…

097eb8f

…ity.aws into feature/elb-security-group

return response

5acc36a

Co-authored-by: Mark Chappell <mchappel@redhat.com>

Merge branch 'feature/elb-security-group' of github.com:mmoyle/commun…

76cd0e5

…ity.aws into feature/elb-security-group

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Sep 22, 2020

mmoyle added 2 commits October 7, 2020 15:09

add check_mode support

5728008

add changelog fragment

d8e35da

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci CI is older than 7 days, rerun before merging labels Nov 16, 2020

gravesm removed the needs_triage label Feb 12, 2021

tremble closed this Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add module elb_security_group and tests #182

Add module elb_security_group and tests #182

mmoyle commented Aug 10, 2020

ansibullbot commented Aug 19, 2020

tremble left a comment

tremble Sep 15, 2020

mmoyle Oct 1, 2020

tremble Sep 15, 2020

tremble Sep 15, 2020

ansibullbot commented Nov 16, 2020

ansibullbot commented Nov 16, 2020

tremble commented Jul 7, 2022

Add module elb_security_group and tests #182

Add module elb_security_group and tests #182

Conversation

mmoyle commented Aug 10, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Aug 19, 2020

tremble left a comment

Choose a reason for hiding this comment

tremble Sep 15, 2020

Choose a reason for hiding this comment

mmoyle Oct 1, 2020

Choose a reason for hiding this comment

tremble Sep 15, 2020

Choose a reason for hiding this comment

tremble Sep 15, 2020

Choose a reason for hiding this comment

ansibullbot commented Nov 16, 2020

ansibullbot commented Nov 16, 2020

tremble commented Jul 7, 2022