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

New module: aci_access_port_block_to_access_port and deprecations in aci_access_port_to_interface_policy_leaf_profile #46182

Merged
merged 5 commits into from
Sep 28, 2018
Merged

New module: aci_access_port_block_to_access_port and deprecations in aci_access_port_to_interface_policy_leaf_profile #46182

merged 5 commits into from
Sep 28, 2018

Conversation

smnmtzgr
Copy link
Contributor

SUMMARY

A new Cisco ACI module is needed, this fixes #43106

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

aci_access_port_block_to_access_port

ANSIBLE VERSION

Goal is to add the new module and also the deprecation information with Ansible 2.8. Deprecation will be effective with Ansible 2.12

ADDITIONAL INFORMATION

Like discussed in the Issue #43106 this new module is needed so that we can also safely delete things (e.g. port blocks) without deleting a complete Port Selector/Access Port. We don`t have 1:1 relationship between a port selector and an access port block. Its 1:x !

Copy link
Contributor

@dagwieers dagwieers left a comment

Choose a reason for hiding this comment

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

We need integration tests now, but the module itself looks fine.

leaf_interface_profile=dict(type='str', aliases=['leaf_interface_profile_name']), # Not required for querying all objects
access_port_selector=dict(type='str', aliases=['name', 'access_port_selector_name']), # Not required for querying all objects
leaf_port_blk=dict(type='str', aliases=['leaf_port_blk_name']), # Not required for querying all objects
from_port=dict(type='str', aliases=['from', 'fromPort', 'from_port_range']), # Not required for querying all objects and deleting port blocks
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not in favor of having fromPort and toPort in there. As we use snake_case in Ansible.

Copy link
Contributor

Choose a reason for hiding this comment

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

If any existing modules do this, it's a backward compatible thing.

Copy link
Contributor Author

@smnmtzgr smnmtzgr Sep 26, 2018

Choose a reason for hiding this comment

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

yep you are right. But they are also in aci_access_port_to_interface_policy_leaf_profile thats why I added them here also. So should we delete them in both modules?

@@ -39,29 +39,53 @@
- The description to assign to the C(access_port_selector)
leaf_port_blk:
description:
- B(Deprecated)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this way of deprecating parameters. In fact the parameters can be deprecated by adding something to the argument_spec. And what we usually do is not mention the parameters in the documentation so people won't start using it (the people still using it get a deprecation warning).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So it would be better to remove them completely from the docstring. But can you give me some more hints how exactly you think it would be done then (especially the deprecation warning)?

@ansibot
Copy link
Contributor

ansibot commented Sep 26, 2018

@ansibot
Copy link
Contributor

ansibot commented Sep 26, 2018

Hi @smnmtzgr,

Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for network.
You can find other people interested in this in #ansible-network on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot ansibot added aci Cisco ACI community affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 26, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 26, 2018
@ansibot
Copy link
Contributor

ansibot commented Sep 26, 2018

@ansibot ansibot added docs This issue/PR relates to or includes documentation. test This PR relates to tests. and removed ci_verified Changes made in this PR are causing tests to fail. labels Sep 26, 2018
@dagwieers
Copy link
Contributor

@rsmeyers @nilloBE Please review !

@rsmeyers
Copy link

Looks ok

@dagwieers
Copy link
Contributor

@smnmtzgr Thanks for your contribution, this will ship with Ansible v2.8 !

@dagwieers
Copy link
Contributor

rebuild_merge

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 28, 2018
@dagwieers dagwieers merged commit 00d78d7 into ansible:devel Sep 28, 2018
@dagwieers dagwieers added the cisco Cisco technologies label Feb 23, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aci Cisco ACI community affects_2.8 This issue/PR affects Ansible v2.8 cisco Cisco technologies core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. networking Network category new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aci_access_port_to_interface_policy_leaf_profile reports changed when child infraPortBlk exist
5 participants