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

Update na_ontap_volume.py #56207

Closed
wants to merge 1 commit into from
Closed

Update na_ontap_volume.py #56207

wants to merge 1 commit into from

Conversation

jontnetapp
Copy link

Specify the valid choices and default for the type parameter

+label: docsite_pr

SUMMARY

Added the specific options available for the "type" field

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

na_ontap_volume

ADDITIONAL INFORMATION

Current documentation is not clear which syntax should be used, as "read-write" and "data-protection" are mentioned as the valid options and the Choices field is blank. I'm just filling in the choices field with the options that actually work correctly (after first using "data-protection" instead of "dp" in my playbook and getting errors!)

<!--- Your description here -->

Specify the valid choices and default for the type parameter

+label: docsite_pr
@ansibot
Copy link
Contributor

ansibot commented May 8, 2019

@ansibot ansibot added affects_2.9 This issue/PR affects Ansible v2.9 committer_review In order to be merged, this PR must follow the certified review workflow. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. netapp new_contributor This PR is the first contribution by a new community member. small_patch storage support:certified This issue/PR relates to certified code. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels May 8, 2019
@acozine acozine removed the needs_triage Needs a first human triage before being processed. label May 9, 2019
@thedoubl3j
Copy link
Member

@lonico @carchi8py something you guys want to take a look at?

@carchi8py
Copy link
Contributor

This is going to be a hard one.
Depending on the version of ONTAP these options are different. The oldest version we support just has 'rw', 'dp', the most common version user are on supports, 'rw', 'dp', 'ls', 'dc'. And i think the latest version removed one of these.
So we'll have to go with String for this.

I'll make an internal story for the team to list which are the valid option for each version of ontap in the documentation.

@thedoubl3j
Copy link
Member

sounds good @carchi8py

@jontnetapp
Copy link
Author

jontnetapp commented May 10, 2019 via email

@carchi8py
Copy link
Contributor

In the 9.3 documentation, for volume-create zapi, the volume-type variable allows for the following

volume-type |   | string optional | The type of the volume to be created. Possible values:

  • "rw" - read-write volume (default setting),
  • "ls" - load-sharing volume,
  • "dp" - data-protection volume,
  • "dc" - data-cache volume (FlexCache)

In 9.5 it might be only RW, and DP, but we need to support 9.1 to the latest version.

@@ -78,7 +78,9 @@
type:
description:
- The volume type, either read-write (RW) or data-protection (DP).

choices: ['rw', 'dp']
Copy link
Contributor

Choose a reason for hiding this comment

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

As @carchi8py pointed out, the ZAPI documentation shows 4 possible values:

  • 'rw' - read-write volume (default),
  • 'ls' - load-sharing volume,
  • 'dp' - data-protection volume,
  • 'dc' - data-cache volume (FlexCache)

(this is from SDK documentation for 9.5)
even though the CLI allows only 2 values.

I'll check with the product team why there is such an inconsistency.

This is also why I am always conflicted with choices. On one hand, this is a great way to help the end users. On the other hand, we need to support all possible choices for 9.1 to 9.6. And if 9.7 introduces a new value, it will require a change in the module. While a free format fields give a lot of flexibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm starting to wonder if this is a bug in the ZAPI documentation. There are only 2 places where ls and dc are mentioned.

We should try and see if the ZAPI actually accept these values.

Anyway, this is not a critical fix, so it's better to delay until we know for sure this change would not break anything.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 19, 2019
@acozine
Copy link
Contributor

acozine commented May 22, 2019

Module documentation must match the argument spec. Adding a default value and choices to the documentation without updating the code will fail our CI system every time.

You could add to the description field for the type option (something like "check your version of ZAPI for allowable values").

@ansibot
Copy link
Contributor

ansibot commented May 22, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/storage/netapp/na_ontap_volume.py:83:1: W293 blank line contains whitespace

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

lib/ansible/modules/storage/netapp/na_ontap_volume.py:0:0: E324 Argument 'type' in argument_spec defines default as (None) but documentation defines default as ('rw')
lib/ansible/modules/storage/netapp/na_ontap_volume.py:0:0: E326 Argument 'type' in argument_spec defines choices as ([]) but documentation defines choices as (['rw', 'dp'])

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label May 22, 2019
@acozine
Copy link
Contributor

acozine commented Jun 12, 2019

Here's what I'd suggest: change the description of the type option to point to the ZAPI documentation. I don't know the best URL, so you shouldn't copy this verbatim, but something like this:

  type:
    description:
    - The volume type. Available values depend on the ZAPI version you target.
    - See U(https://docs.netapp.com/ontap-9/index.jsp) for more information.
   type: string
   default: None

This way the documentation of the Ansible module is correct, and users have the right resources to figure out which values they should use.

@acozine
Copy link
Contributor

acozine commented Jun 12, 2019

Also, you could add a line in the description field for the module itself (line 26 of this file) documenting which versions of your API the module supports.

@jontnetapp
Copy link
Author

jontnetapp commented Jun 14, 2019 via email

@JohnLieske
Copy link

Per request, closing this PR & maintainers will address.

@JohnLieske JohnLieske closed this Jun 21, 2019
@ansible ansible locked and limited conversation to collaborators Aug 5, 2019
@dagwieers dagwieers added the docsite_pr This PR is created from documentation using the "Edit on GitHub" link. label Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.9 This issue/PR affects Ansible v2.9 ci_verified Changes made in this PR are causing tests to fail. docs This issue/PR relates to or includes documentation. docsite_pr This PR is created from documentation using the "Edit on GitHub" link. 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 new_contributor This PR is the first contribution by a new community member. small_patch stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. storage support:certified This issue/PR relates to certified code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants