aci_tenant: New module to manage tenants on ACI fabric #26836

schunduri · 2017-07-15T00:43:16Z

SUMMARY

Cisco ACI Module

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

ANSIBLE VERSION

ansible 2.4.0 (devel 489aefe807) last updated 2017/07/14 17:43:27 (GMT -700)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/user/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/user/ansible/lib/ansible
  executable location = /home/user/ansible/bin/ansible
  python version = 2.7.6 (default, Jun 22 2015, 17:58:13) [GCC 4.8.2]

ADDITIONAL INFORMATION

ansibot · 2017-07-19T16:11:26Z

@schunduri this PR contains the following merge commits:

dec252e

Please rebase your branch to remove these commits.

click here for bot help

ansibot · 2017-07-19T16:45:47Z

@schunduri this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

ansibot · 2017-07-19T16:55:00Z

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/network/aci/aci_tenant.py:0:0: E305 DOCUMENTATION.version_added: required key not provided @ data['version_added']. Got None
lib/ansible/modules/network/aci/aci_tenant.py:0:0: E307 version_added should be 2.4. Currently 0.0

click here for bot help

ansibot · 2017-07-19T23:31:34Z

The test ansible-test sanity --test validate-modules failed with the following errors:

lib/ansible/modules/network/aci/aci_tenant.py:0:0: E305 DOCUMENTATION.version_added: required key not provided @ data['version_added']. Got None
lib/ansible/modules/network/aci/aci_tenant.py:0:0: E307 version_added should be 2.4. Currently 0.0

click here for bot help

ansibot · 2017-07-20T00:18:12Z

@jedelman8 @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

gundalow · 2017-07-17T14:52:23Z

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

+---
+
+module: aci_tenant
+short_description: Direct access to the APIC API


Can you please add "cisco" and "ACI" into the short_description, it will make it easier for people to find.
Direct access to the Cisco ACI APIC API
Feel free to change the wording to match how you market this.

gundalow · 2017-07-24T13:31:30Z

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

+    hostname=dict(type='str', required=True, aliases=['host']),
+    username=dict(type='str', default='admin', aliases=['user']),
+    password=dict(type='str', required=True, no_log=True),
+    protocol=dict(type='str'),  # Deprecated in v2.8


Please change to:
protocol=dict(type='str', removed_in_version='2.6'), # Deprecated in v2.6

So this matches #27070

gundalow · 2017-07-24T13:31:48Z

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

+
+import json
+
+# from ansible.module_utils.aci import aci_argument_spec, aci_login, aci_response


please remove commented out code

gundalow · 2017-07-24T13:31:59Z

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

+    username = module.params['username']
+    password = module.params['password']
+
+    # FIXME: This should not be needed


@gundalow I will make the required changes. Thank you

gundalow · 2017-07-24T13:32:55Z

@dagwieers Did you want to review this?

alikins · 2017-08-07T16:46:12Z

Please reopen when the git branch issues are resolved.

Having so many changed files in a pr is causing some problems with repo info tools (like https://ansible.sivel.net/byfile.html) so closing for now.

jctanner · 2017-08-07T22:53:46Z

per @dagwieers on irc, this is about to get some rebasing and the submitter would like it reopened.

ansibot · 2017-08-08T00:41:30Z

The test ansible-test sanity --test boilerplate failed with the following error:

Command "test/sanity/code-smell/boilerplate.sh" returned exit status 2.
>>> Standard Output
== Missing __metaclass__ = type ==
./lib/ansible/modules/network/aci/aci_tenant.py

== Missing from __future__ import (absolute_import, division, print_function) ==
./lib/ansible/modules/network/aci/aci_tenant.py

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:0:0: E101 Interpreter line is not "#!/usr/bin/python"

click here for bot help

ansibot · 2017-08-08T02:38:03Z

The test ansible-test sanity --test import --python 2.6 failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:6:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 2.7 failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:6:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 3.5 failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:6:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 3.6 failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:6:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/network/aci/aci_tenant.py:0:0: E101 Interpreter line is not "#!/usr/bin/python"

click here for bot help

Review points made have been resolved

jedelman8 · 2017-08-09T13:25:12Z

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

+    if state == 'query':
+        aci.request(path)
+    elif module.check_mode:
+        # TODO: Implement proper check-mode (presence check)


Hi @calfonso shouldn't we want a real check mode if the module says it supports it? This blindly returns changed=True in check mode all the time.

/cc @dagwieers @devarshishah3

@jedelman8 you're right. @schunduri ^

Yes, so we plan to fix this and @jedelman8 and @jmcgill298 are involved in some new code that may help with this. However if this does not hit the tree before the freeze I will make sure this module is pulled from the tree.

It is unfortunate this PR was merged, but granted the PR should have been indicated with a WIP-tag to have prevented this.

I have submitted a couple of modules with the new approach that takes care of check-mode. I am waiting to hear back from @schunduri that this approach is acceptable before continuing to enhance other modules being developed.

@jedelman8, we can change the module to check mode not supported for now

bcoca added support:community This issue/PR relates to code supported by the Ansible community. and removed needs_triage Needs a first human triage before being processed. labels Jul 17, 2017

mattclay added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 18, 2017

ansibot added module This issue/PR relates to a module. networking Network category new_module This PR includes a new module. labels Jul 18, 2017

ansibot added the merge_commit This PR contains at least one merge commit. Please resolve! label Jul 19, 2017

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 19, 2017

schunduri force-pushed the devel branch from f3d509c to 4fb9cb2 Compare July 19, 2017 22:47

ansibot removed ci_verified Changes made in this PR are causing tests to fail. 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 labels Jul 19, 2017

ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jul 19, 2017

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 Jul 20, 2017

gundalow previously requested changes Jul 24, 2017

View reviewed changes

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

gundalow removed virt Virt community (incl. QEMU, KVM, libvirt, ovirt, RHV and Proxmox) vmware VMware community labels Aug 4, 2017

alikins closed this Aug 7, 2017

jctanner reopened this Aug 7, 2017

schunduri closed this Aug 7, 2017

schunduri force-pushed the devel branch from 3232a08 to 5df4ff8 Compare August 7, 2017 23:21

schunduri reopened this Aug 7, 2017

schunduri force-pushed the devel branch from bd62ca9 to 49d1b74 Compare August 7, 2017 23:59

New ACI Tenant module

49d1b74

ansibot added module This issue/PR relates to a module. new_module This PR includes a new module. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Aug 8, 2017

New module requirements added.

72a758b

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Aug 8, 2017

Placed from __future__ import to be the first line

17f6dfd

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Aug 8, 2017

Interpreter line added

d3a0070

calfonso merged commit 3241b71 into ansible:devel Aug 9, 2017

jedelman8 reviewed Aug 9, 2017

View reviewed changes

dagwieers changed the title ~~Cisco ACI New Module PR~~ aci_tenant: New module to manage tenants on ACI fabric Aug 11, 2017

jmcgill298 mentioned this pull request Aug 11, 2017

aci_tenant: Update to support check_mode #28090

Merged

dagwieers added the aci Cisco ACI community label Sep 27, 2017

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

aci_tenant: New module to manage tenants on ACI fabric #26836

aci_tenant: New module to manage tenants on ACI fabric #26836

schunduri commented Jul 15, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 20, 2017

gundalow Jul 17, 2017

gundalow Jul 24, 2017

gundalow Jul 24, 2017

gundalow Jul 24, 2017

schunduri Jul 24, 2017

gundalow commented Jul 24, 2017

alikins commented Aug 7, 2017

jctanner commented Aug 7, 2017

ansibot commented Aug 8, 2017

ansibot commented Aug 8, 2017

jedelman8 Aug 9, 2017

calfonso Aug 9, 2017

dagwieers Aug 9, 2017

jmcgill298 Aug 9, 2017

devarshishah3 Aug 9, 2017


		import json

		# from ansible.module_utils.aci import aci_argument_spec, aci_login, aci_response

aci_tenant: New module to manage tenants on ACI fabric #26836

aci_tenant: New module to manage tenants on ACI fabric #26836

Conversation

schunduri commented Jul 15, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 19, 2017

ansibot commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gundalow commented Jul 24, 2017

alikins commented Aug 7, 2017

jctanner commented Aug 7, 2017

ansibot commented Aug 8, 2017

ansibot commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment