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.

Sign up for GitHub

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

Jump to bottom

Fix unwanted deprecation message in network module args #28984

Merged
merged 3 commits into from
Sep 12, 2017
Merged

Fix unwanted deprecation message in network module args #28984

merged 3 commits into from
Sep 12, 2017

Conversation

ganeshrn
Copy link
Member

@ganeshrn ganeshrn commented Sep 4, 2017 •
edited
Loading

SUMMARY

Fixes #25663
Fixes #24537

  • segregate provider spec and top level arg spec
  • add depreciation key in top level arg spec and ensure warning is not thrown when
    the corresponding environment variable is set
  • remove action plugin code to load provider and add
    that logic at a common place in network_common.py file
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

module_utils/aireos.py
module_utils/aruba.py
module_utils/asa.py
module_utils/ce.py
module_utils/dellos10.py
module_utils/dellos6.py
module_utils/dellos9.py
module_utils/eos.py
module_utils/ios.py
module_utils/iosxr.py
module_utils/junos.py
module_utils/network_common.py
module_utils/nxos.py
module_utils/sros.py
plugins/action/aireos.py
plugins/action/aruba.py
plugins/action/asa.py
plugins/action/ce.py
plugins/action/dellos10.py
plugins/action/dellos6.py
plugins/action/dellos9.py
plugins/action/eos.py
plugins/action/ios.py
plugins/action/iosxr.py
plugins/action/junos.py
plugins/action/net_base.py
plugins/action/nxos.py
plugins/action/sros.py
plugins/action/vyos.py

ANSIBLE VERSION
2.4
ADDITIONAL INFORMATION

Before

TASK [Get Version] ***************************************************************************************************************************************************************************************************************************
 [WARNING]: argument username has been deprecated and will be removed in a future version

 [WARNING]: argument password has been deprecated and will be removed in a future version

 [WARNING]: argument transport has been deprecated and will be removed in a future version

After
(In case environment variable for password, username etc is set and top level argument value is not given in playbook, corresponding warning will not be displayed in output. )

[DEPRECATION WARNING]: Param 'host' is deprecated. See the module docs for more information.
This feature will be
removed in version 2.3. Deprecation warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.
[DEPRECATION WARNING]: Param 'password' is deprecated. See the module docs for more information.
This feature will
be removed in version 2.3. Deprecation warnings can be disabled by setting deprecation_warnings=False in
ansible.cfg.

@ganeshrn
Fix unwanted deprecation message in network module argspec
badc0fa 
Fixes ansible#25663
Fixes ansible#24537

*  segregate provider spec and top level arg spec
*  add deprecation key in top level arg spec
*  remove action plugin code to load provider and add
   that logic at a common place in network_common.py file
@ganeshrn ganeshrn added this to the 2.4.0 milestone Sep 4, 2017
@ganeshrn ganeshrn changed the title Fix unwanted deprecation message in network module argspec Fix unwanted deprecation message in network module args Sep 4, 2017
@ansibot
Copy link
Contributor

ansibot commented Sep 4, 2017

cc @jmighion @ogenstad @skg-net @Qalthos @gundalow @privateip @rcarrillocruz @trishnaguha
click here for bot help

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 bugfix_pull_request module_utils/ needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. networking Network category plugins/action support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 4, 2017
@ansible ansible deleted a comment from ansibot Sep 4, 2017
@ansible ansible deleted a comment from ansibot Sep 4, 2017
@ansible ansible deleted a comment from ansibot Sep 4, 2017
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Sep 4, 2017
@ganeshrn ganeshrn removed the needs_triage Needs a first human triage before being processed. label Sep 5, 2017
@Qalthos
Copy link
Contributor

Qalthos commented Sep 5, 2017

Maybe I missed it skimming the changes, but why split $NOS_argument_spec and $NOS_top_spec and not just have $NOS_argument_spec?

@ganeshrn
Copy link
Member Author

ganeshrn commented Sep 7, 2017 •
edited
Loading

@Qalthos The top level arguments like host, port, username etc are deprecated and currently warning message is thrown in check_args() function if these args are present at a top level in play.

However in $NOS_argument_spec there is a fallback for environment variable for these args. So if env variable is set, for current implementation it will end up throwing a warning message even if the arg is not mentioned in play which is not the expected behavior (refer the linked issues in summary for more details).

With a separate spec $NOS_top_spec there is no environment fall back so now warning will be shown only when these deprecated args are present at the top level of a task. Also, the removed_in_version for these args avoids the hack in check_args()

The fallback environment variable in $NOS_provider_spec will map to the respective argument inside provider spec without any deprecation warning in output.

gundalow
gundalow reviewed Sep 7, 2017
View reviewed changes
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Transport, authorize don't consistently have removed_in_version.

lib/ansible/module_utils/asa.py
'username': dict(removed_in_version=2.3),
'password': dict(removed_in_version=2.3, no_log=True),
'ssh_keyfile': dict(removed_in_version=2.3, type='path'),
'authorize': dict(type='bool'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is removed_in_version missing from this?

Copy link
Member Author

Choose a reason for hiding this comment

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

In current implementation warning in not thrown if authorize is mentioned at top level.
To keep the same behavior after refactor removed_in_version is not added for authorize

Copy link
Member Author

Choose a reason for hiding this comment

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

asa_config doc here confirms the same.

lib/ansible/module_utils/ce.py
'use_ssl': dict(removed_in_version=2.3, type='bool'),
'validate_certs': dict(removed_in_version=2.3, type='bool'),
'timeout': dict(removed_in_version=2.3, type='int'),
'transport': dict(choices=['cli']),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this deprecated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reason as above.

@trishnaguha
Copy link
Member

trishnaguha commented Sep 8, 2017 •
edited
Loading

Segregating top level and provider level argspec makes sense to me. LGTM overall.

@ganeshrn ganeshrn mentioned this pull request Sep 11, 2017
Closed
@rcarrillocruz rcarrillocruz merged commit 599fe23 into ansible:devel Sep 12, 2017
@ganeshrn ganeshrn deleted the provider_warnings branch September 12, 2017 12:06
prasadkatti pushed a commit to prasadkatti/ansible that referenced this pull request Oct 1, 2017
@ganeshrn
Fix unwanted deprecation message in network module args (ansible#28984)
c416d8c 
* Fix unwanted deprecation message in network module argspec

Fixes ansible#25663
Fixes ansible#24537

*  segregate provider spec and top level arg spec
*  add deprecation key in top level arg spec
*  remove action plugin code to load provider and add
   that logic at a common place in network_common.py file

* Fix CI issue

* Minor change
BondAnthony pushed a commit to BondAnthony/ansible that referenced this pull request Oct 5, 2017
@ganeshrn @BondAnthony
Fix unwanted deprecation message in network module args (ansible#28984)
412c845 
* Fix unwanted deprecation message in network module argspec

Fixes ansible#25663
Fixes ansible#24537

*  segregate provider spec and top level arg spec
*  add deprecation key in top level arg spec
*  remove action plugin code to load provider and add
   that logic at a common place in network_common.py file

* Fix CI issue

* Minor change
ganeshrn added a commit to ganeshrn/ansible that referenced this pull request Oct 31, 2017
@ganeshrn
Move asa provider to suboptions
e10e2f0 
Fixes ansible#32343

* Move provider arg spec as part of suboptions
  to validate input args against provider spec.
* This handles `no_log` for password arg correctly.

Merged to devel PR ansible#28984

( cherry picked from commit 599fe23 )
@ganeshrn ganeshrn mentioned this pull request Oct 31, 2017
Merged
ganeshrn added a commit that referenced this pull request Nov 1, 2017
@ganeshrn
Move asa provider to suboptions (#32356)
8c1dfdb 
Fixes #32343

* Move provider arg spec as part of suboptions
  to validate input args against provider spec.
* This handles `no_log` for password arg correctly.

Merged to devel PR #28984

( cherry picked from commit 599fe23 )
@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018
@dagwieers dagwieers added nxos Cisco NXOS community asa Cisco ASA community cisco Cisco technologies labels Feb 22, 2019
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gundalow gundalow gundalow left review comments

@trishnaguha trishnaguha trishnaguha approved these changes

@privateip privateip Awaiting requested review from privateip

@rcarrillocruz rcarrillocruz Awaiting requested review from rcarrillocruz

@Qalthos Qalthos Awaiting requested review from Qalthos

Assignees
No one assigned
Labels
affects_2.4 This issue/PR affects Ansible v2.4 asa Cisco ASA community bug This issue/PR relates to a bug. cisco Cisco technologies module_utils/ networking Network category nxos Cisco NXOS community plugins/action support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
2.4.0
7 participants
@ganeshrn @ansibot @Qalthos @trishnaguha @gundalow @dagwieers @rcarrillocruz