added aci_interface_policy_leaf_policy_group module #34968

brunocalogero · 2018-01-17T02:46:39Z

SUMMARY

Adding aci_interface_policy_leaf_policy_group module, creates the following ACI objects: infra:AccPortGrp and infra:AccBndlGrp

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

aci_interface_policy_leaf_policy_group

ANSIBLE VERSION

2.5

ansibot · 2018-01-17T02:50:02Z

cc @dagwieers @jmcgill298 @schunduri
click here for bot help

sivel · 2018-01-17T20:53:18Z

lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py

+
+
+def main():
+    argument_spec = aci_argument_spec


aci_argument_spec has been updated to be a function. This line should be updated as:

argument_spec = aci_argument_spec()

alrighty ! 👍

brunocalogero · 2018-01-17T22:03:21Z

2018-01-17 21:33:18 ERROR: Found 14 validate-modules issue(s) which need to be resolved: 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "cert_key" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "cert_name" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "certificate_name" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "host" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "hostname" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "password" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "private_key" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "protocol" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "timeout" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "use_proxy" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "use_ssl" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "user" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "username" is listed in the argument_spec, but not documented in the module (75%) 2018-01-17 21:33:18 ERROR: lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py:0:0: E322 "validate_certs" is listed in the argument_spec, but not documented in the module (75%), are there new documentation rules ?

sivel · 2018-01-17T22:06:36Z

are there new documentation rules

Yes, the new rules were activated today after testing.

The errors indicate inconsistencies between the DOCUMENTATION and the argument_spec. Looking at those errors, it may indicate at a minimum that you are missing a extends_documentation_fragment in your DOCUMENTATION.

The bot should likely also be updating this PR with a comment soon in regards to those errors.

brunocalogero · 2018-01-17T22:50:50Z

@sivel hey what do you suggest to fix the protocol documentation error ? what are the possible options for protocol, strange that adding extends_documentation_fragment: aci fixed everything except this one

sivel · 2018-01-17T23:18:10Z

@dagwieers is protocol one of the arguments that is purposefully not documented?

…w optional description

dagwieers · 2018-01-18T19:17:21Z

@sivel Yes, it's replaced with use_ssl (in line with other modules). It's now fixed as we moved protocol out of the standard aci_argument_spec since it's being deprecated and only used for v2.4 modules, not the new stuff.

dagwieers · 2018-01-18T20:33:19Z

lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py

+ attached_entity_profile:
+   description:
+   - Choice of attached_entity_profile (AEP) to be used as part of the leaf policy group to be created.
+   aliases: [ attached_entity_profile_name, AEP, AEP_name ]


We use aep as the default parameter in other modules and we never used AEP, AEP_name or attached_entity_profile. So please use the same parameter name as the aci_aep module !

dagwieers · 2018-01-18T20:34:54Z

lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py

+ link_aggregation_type:
+   description:
+   - Selector for the type of leaf policy group we want to create.
+   aliases: [ link_aggregation_type_name ]


I wonder if this wouldn't be better named lag_type either as the default name, or an alias. As LAG seems to be a common term (at least I know this term).
cc @rsmeyers @jmcgill298

agreed, was just wondering if we should make it same as GUI or not 👍

dagwieers · 2018-01-18T20:36:53Z

lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py

+- Bruno Calogero (@brunocalogero)
+version_added: '2.5'
+notes:
+- When using the module please select the appropriate link_aggregation_type. 'link'(PC)/'node'(VPC), 'leaf'(Leaf Access Port Policy Group)


You should be doing: C(link) for PC, C(node) for VPC and C(leaf) for Leaf Access Port Policy Group.

Maybe also use the full name for PC and VPC as well ?

dagwieers · 2018-01-18T20:37:34Z

lib/ansible/modules/network/aci/aci_interface_policy_leaf_policy_group.py

+    argument_spec = aci_argument_spec()
+    argument_spec.update({
+        'policy_group': dict(type='str', aliases=['name', 'policy_group_name']),
+        'description': dict(type='str', aliases=['descr', 'description']),


Please remove `description' from the aliases.

dagwieers

LGTM

brunocalogero · 2018-01-19T19:33:56Z

@sivel can you approve changes ?

sivel · 2018-01-19T19:55:50Z

I've approved the changes I asked for. I'll leave this in @dagwieers' hands, as I have no expertise in this specific area (aci).

ansibot · 2018-01-19T19:58:07Z

@jmcgill298 @schunduri

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

dagwieers · 2018-01-19T20:31:52Z

I made one more commit to it, but lost track waiting for it to be validated.

dagwieers · 2018-01-19T20:32:07Z

PS Please don't forget to add the integration test in due time.

ansible deleted a comment from ansibot Jan 17, 2018

sivel removed the needs_triage Needs a first human triage before being processed. label Jan 17, 2018

sivel requested changes Jan 17, 2018

View reviewed changes

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jan 17, 2018

bcaloger added 5 commits January 18, 2018 10:35

added aci_interface_policy_leaf_policy_group module

ab08a92

fixing documentation errors

c5b66f4

fixed argument_spec function and fixing documentation errors

f463e06

added documentation extends..

334f233

removed unnecessary aliases, improved notes, improved examples to sho…

f310252

…w optional description

brunocalogero force-pushed the aci_interface_policy_leaf_policy_group branch from 3e895cf to f310252 Compare January 18, 2018 18:51

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jan 18, 2018

ansible deleted a comment from ansibot Jan 18, 2018

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jan 18, 2018

fixing sanity checks

408f4cd

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jan 18, 2018

brunocalogero changed the title ~~WIP: added aci_interface_policy_leaf_policy_group module~~ added aci_interface_policy_leaf_policy_group module Jan 18, 2018

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Jan 18, 2018

ansible deleted a comment from ansibot Jan 18, 2018

dagwieers suggested changes Jan 18, 2018

View reviewed changes

fixed proposed changes by dag

850246b

dagwieers approved these changes Jan 19, 2018

View reviewed changes

Fix one more leftover

3f54723

sivel approved these changes Jan 19, 2018

View reviewed changes

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 19, 2018

dagwieers merged commit 04f1b14 into ansible:devel Jan 19, 2018

dagwieers added the cisco Cisco technologies label Feb 23, 2019

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added aci_interface_policy_leaf_policy_group module #34968

added aci_interface_policy_leaf_policy_group module #34968

brunocalogero commented Jan 17, 2018

ansibot commented Jan 17, 2018

sivel Jan 17, 2018

brunocalogero Jan 17, 2018

brunocalogero commented Jan 17, 2018

sivel commented Jan 17, 2018

brunocalogero commented Jan 17, 2018

sivel commented Jan 17, 2018

dagwieers commented Jan 18, 2018 •

edited

Loading

dagwieers Jan 18, 2018

dagwieers Jan 18, 2018

brunocalogero Jan 18, 2018

dagwieers Jan 18, 2018

dagwieers Jan 18, 2018

dagwieers left a comment

brunocalogero commented Jan 19, 2018

sivel commented Jan 19, 2018

ansibot commented Jan 19, 2018

dagwieers commented Jan 19, 2018

dagwieers commented Jan 19, 2018

added aci_interface_policy_leaf_policy_group module #34968

added aci_interface_policy_leaf_policy_group module #34968

Conversation

brunocalogero commented Jan 17, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Jan 17, 2018

sivel Jan 17, 2018

Choose a reason for hiding this comment

brunocalogero Jan 17, 2018

Choose a reason for hiding this comment

brunocalogero commented Jan 17, 2018

sivel commented Jan 17, 2018

brunocalogero commented Jan 17, 2018

sivel commented Jan 17, 2018

dagwieers commented Jan 18, 2018 • edited Loading

dagwieers Jan 18, 2018

Choose a reason for hiding this comment

dagwieers Jan 18, 2018

Choose a reason for hiding this comment

brunocalogero Jan 18, 2018

Choose a reason for hiding this comment

dagwieers Jan 18, 2018

Choose a reason for hiding this comment

dagwieers Jan 18, 2018

Choose a reason for hiding this comment

dagwieers left a comment

Choose a reason for hiding this comment

brunocalogero commented Jan 19, 2018

sivel commented Jan 19, 2018

ansibot commented Jan 19, 2018

dagwieers commented Jan 19, 2018

dagwieers commented Jan 19, 2018

dagwieers commented Jan 18, 2018 •

edited

Loading