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

Added folder and datacenter to the examples #25221

Merged
merged 2 commits into from
Aug 28, 2017
Merged

Added folder and datacenter to the examples #25221

merged 2 commits into from
Aug 28, 2017

Conversation

Tronde
Copy link
Contributor

@Tronde Tronde commented May 31, 2017

SUMMARY

From #22644 it seems that the parameter 'folder' is mandatory. While both are required parameters they should be used in the examples.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

vmware_guest_snapshot

ANSIBLE VERSION
ansible 2.3.0.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = Default w/o overrides
  python version = 2.7.5 (default, Aug  2 2016, 04:20:16) [GCC 4.8.5 20150623 (Red Hat 4.8.5-4)]

@Tronde
Added folder and datacenter to the examples
a9ef725 
From #22644 it seems that the parameter 'folder' is mandatory. While both are required parameters they should be used in the examples.
@ansibot
Copy link
Contributor

ansibot commented May 31, 2017

cc @jctanner @nerzhul
click here for bot help

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 cloud community_review In order to be merged, this PR must follow the community review workflow. docs_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. vmware VMware community labels May 31, 2017
gundalow
gundalow reviewed Jun 1, 2017
View reviewed changes
lib/ansible/modules/cloud/vmware/vmware_guest_snapshot.py Outdated
@@ -60,6 +60,7 @@
folder:
description:
- Define instance folder location.
required: True
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't look correct
folder=dict(required=False, type='str', default='/vm'),

I think it should actually be
default: /vm on this line

Copy link
Member

Choose a reason for hiding this comment

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

this is not a documentation issue, module should be changed to either require folder or implement an actual search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the issue with the folder parameter may not be a documentation issue in the first place. I'm also not sure if it makes much sense to define a default value for an optional parameter. Maybe that discussion should stay in #22644.

I suggest to remove line 63 again, but leave the examples as they are. Showing the option in the examples don't hurt. Would you agree on that?

Copy link
Member

Choose a reason for hiding this comment

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

optional parameters should always have a default, the module does not need to use it though.

A saner default would have been 'null/None' in which case the module should ignore it.

@gundalow gundalow removed the needs_triage Needs a first human triage before being processed. label Jun 1, 2017
@Tronde
Removed 'required: True' from folder documentation
fd01b51 
I reverted the documentation of the folder option to be a not required one. This way it stays consistent with the definition of the function.
@Tronde
Copy link
Contributor Author

Tronde commented Jun 1, 2017

So I reverted the change to the folder documentation to be a non required option again. This way the documentation is consistent to the code, where it is defined as not required as well:
folder=dict(required=False, type='str', default='/vm'),

This way the discussion about how to change this option could take place outside of this PR.

The parameter 'datacenter' should be used in the examples, because it is a mandatory one. This way the examples are consistent with the parameter definition at the beginning of the module documentation. The 'folder' parameter should be used in the examples as well. It doesn't hurt, whether it is required or not. And in the current version you have to use it or your vm won't be found.

@Tronde
Copy link
Contributor Author

Tronde commented Jun 4, 2017

This PR would fix #24203.

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. labels Jun 23, 2017
@ansibot
Copy link
Contributor

ansibot commented Jul 18, 2017

cc @Akasurde @dagwieers @dav1x
click here for bot help

Akasurde
Akasurde approved these changes Jul 23, 2017
View reviewed changes
Copy link
Member

@Akasurde Akasurde left a comment

Choose a reason for hiding this comment

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

shipit

@Akasurde Akasurde mentioned this pull request Jul 27, 2017
Closed
@dagwieers dagwieers merged commit f08c299 into ansible:devel Aug 28, 2017
@dav1x
Copy link
Contributor

dav1x commented Aug 28, 2017

shipit

@Tronde Tronde deleted the patch-1 branch August 29, 2017 08:08
prasadkatti pushed a commit to prasadkatti/ansible that referenced this pull request Oct 1, 2017
@Tronde
Added folder and datacenter to the examples (ansible#25221)
e47f89b 
* Added folder and datacenter to the examples

From ansible#22644 it seems that the parameter 'folder' is mandatory. While both are required parameters they should be used in the examples.

* Removed 'required: True' from folder documentation

I reverted the documentation of the folder option to be a not required one. This way it stays consistent with the definition of the function.
@ansibot ansibot added docs This issue/PR relates to or includes documentation. and removed docs_pull_request labels Mar 4, 2018
@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

@bcoca bcoca bcoca left review comments

@gundalow gundalow gundalow left review comments

@Akasurde Akasurde Akasurde approved these changes

Assignees
No one assigned
Labels
affects_2.4 This issue/PR affects Ansible v2.4 cloud community_review In order to be merged, this PR must follow the community review workflow. docs This issue/PR relates to or includes documentation. module This issue/PR relates to a module. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@Tronde @ansibot @dav1x @Akasurde @bcoca @gundalow @dagwieers