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

Fixed netapp_e_lun_mapping options for backwards compatibilitiy #44769

Merged
merged 2 commits into from
Aug 30, 2018
Merged

Fixed netapp_e_lun_mapping options for backwards compatibilitiy #44769

merged 2 commits into from
Aug 30, 2018

Conversation

ndswartz
Copy link
Contributor

SUMMARY

Readded options lun and target_type which should have been deprecated instead of being removed.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

netapp_e_lun_mapping

ANSIBLE VERSION
ansible 2.7.0.dev0 (lun_mapping_pub_update faac31d6e6) last updated 2018/08/24 15:20:12 (GMT -500)
  config file = None
  configured module search path = [u'/home/swartzn/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/swartzn/ansible-dev/lib/ansible
  executable location = /home/swartzn/ansible-dev/bin/ansible
  python version = 2.7.15rc1 (default, Apr 15 2018, 21:51:34) [GCC 7.3.0]

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

cc @amit0701 @carchi8py @hulquest @lmprice @schmots1
click here for bot help

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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. netapp owner_pr This PR is made by the module's maintainer. storage support:community This issue/PR relates to code supported by the Ansible community. labels Aug 28, 2018
@webknjaz webknjaz requested a review from alikins August 28, 2018 14:46
@webknjaz webknjaz removed the needs_triage Needs a first human triage before being processed. label Aug 28, 2018
@ndswartz ndswartz closed this Aug 28, 2018
@ndswartz ndswartz reopened this Aug 28, 2018
gundalow
gundalow requested changes Aug 28, 2018
View reviewed changes
lib/ansible/modules/storage/netapp/netapp_e_lun_mapping.py Outdated
@@ -89,7 +103,9 @@ def __init__(self):
argument_spec.update(dict(
state=dict(required=True, choices=["present", "absent"]),
target=dict(required=False, default=None),
volume_name=dict(required=True)))
volume_name=dict(required=True, aliases=["volume"]),
lun=dict(required=False),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this been added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We simply found volume_name to be unnecessarily verbose and thought adding volume as an alias would help.

lib/ansible/modules/storage/netapp/netapp_e_lun_mapping.py Outdated
- volume
lun:
description:
- This option is deprecated and will have no effect on the module's outcome
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused, why has this been added if it's deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous merge removed the two options, lun and target_type, that should have been deprecate instead for backwards capability. How should I correct my mistake?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if these options don't do anything that seems to break backwards compatability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there are existing customer plays that contain the two options, the module will still work as expected but we simplified the logic such that the two options are not needed. The netapp e-series restapi already had the capability to determine those options without customers explicitly stating them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could the existing customer set these deprecated options to something other than what the restapi would automatically do, ie surprise the user?

If these options don't do anything then removing them maybe the right decision. A note: should be added in the he modules docs and in porting_guide_2.7.rst

lib/ansible/modules/storage/netapp/netapp_e_lun_mapping.py Outdated
required: no
target_type:
description:
- This option is deprecated and will have no effect on the module's outcome
Copy link
Contributor

Choose a reason for hiding this comment

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

Why has this been added if it's deprecated?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gundalow There was a patch submitted to resolve a different issue a few days ago. (#44666). This patch removed a few parameters from the module that we realized really weren't necessary. Unfortunately, removing those fields would cause existing playbooks utilizing those parameters to break (that's my fault for not remembering that). This patch is adding those back in (as deprecated fields), to maintain existing playbooks. Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

If these options don't do anything then I think it may be confusing for the end user to leave them in but not used

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

@ndswartz this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added the merge_commit This PR contains at least one merge commit. Please resolve! label Aug 28, 2018
@lmprice
Copy link
Contributor

lmprice commented Aug 28, 2018

@ndswartz You'll need to squash this back down to a single commit.

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

@ndswartz this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Aug 28, 2018

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

lib/ansible/modules/storage/netapp/netapp_e_lun_mapping.py:0:0: E309 version_added for new option (lun) should be 2.7. Currently 0.0
lib/ansible/modules/storage/netapp/netapp_e_lun_mapping.py:0:0: E309 version_added for new option (target_type) should be 2.7. Currently 0.0

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Aug 28, 2018
@swartzn
Fixed netapp_e_lun_mapping options for backwards compatibility.
84ddc73 
Readd lun and target_type as deprecated options.

Note: lun and target_type were removed in patch ansible#44666 since they were
no longer needed for the logic in the module.  However, this cause will
cause existing playbooks utilizing these options to break.
@ansibot 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 Aug 28, 2018
@gundalow
Copy link
Contributor

Could the existing customer set these deprecated options to something other than what the restapi would automatically do, ie surprise the user?

If these options don't do anything then removing them maybe the right decision. A note: should be added in the he modules docs and in porting_guide_2.7.rst

Is these options don't done do anything I'm not sure in the value of keeping them.
Keeping options that don't aren't honored feels like it will confuse the user more

@ndswartz
Copy link
Contributor Author

@gundalow After some discussion resulting from your question about surprising the user, we realized that there is a possibility of ambiguity that a host target and a group target could have the same name. Furthermore it is also possible that the customer could specify a host target_type when in reality it is a group target and vice-versa.

So, we have added functionality to both the target_type and the lun options. The target_type option will be optionally used to disambiguate mappable objects and lun will give the user the flexibility to specify the lun value.

@gundalow gundalow merged commit b0cee34 into ansible:devel Aug 30, 2018
@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.
Reviewers

@gundalow gundalow gundalow requested changes

@lmprice lmprice lmprice left review comments

@alikins alikins Awaiting requested review from alikins

Assignees
No one assigned
Labels
affects_2.7 This issue/PR affects Ansible v2.7 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. netapp owner_pr This PR is made by the module's maintainer. storage support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@ndswartz @ansibot @lmprice @gundalow @webknjaz @swartzn