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

VMware: dns_suffix as a list of domain suffixes #42678

Merged
merged 1 commit into from
Jul 17, 2018
Merged

VMware: dns_suffix as a list of domain suffixes #42678

merged 1 commit into from
Jul 17, 2018

Conversation

Akasurde
Copy link
Member

SUMMARY

This fix adds dns_suffix as a list of domain suffixes and
also updates documentation and example.

Fixes: #42229

Signed-off-by: Abhijeet Kasurde akasurde@redhat.com

ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
COMPONENT NAME

lib/ansible/modules/cloud/vmware/vmware_guest.py

ANSIBLE VERSION
2.7-devel

@ansibot
Copy link
Contributor

ansibot commented Jul 12, 2018

cc @dav1x @nerzhul @pdellaert @warthog9
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. cloud community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. owner_pr This PR is made by the module's maintainer. support:community This issue/PR relates to code supported by the Ansible community. vmware VMware community labels Jul 12, 2018
@ansibot

This comment has been minimized.

Sign in to view
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 12, 2018
@Akasurde Akasurde mentioned this pull request Jul 12, 2018
Closed
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 12, 2018
@samdoran samdoran removed the needs_triage Needs a first human triage before being processed. label Jul 12, 2018
@Akasurde Akasurde requested a review from dagwieers July 17, 2018 03:54
@Akasurde
Copy link
Member Author

@dericcrago @pdellaert @dagwieers Could you please review this ?

dagwieers
dagwieers suggested changes Jul 17, 2018
View reviewed changes
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.

Only a single remark.

lib/ansible/modules/cloud/vmware/vmware_guest.py Outdated
@@ -287,7 +287,8 @@
- Linux based OSes requires Perl package to be installed for OS customizations.
- 'Common parameters (Linux/Windows):'
- ' - C(dns_servers) (list): List of DNS servers to configure.'
- ' - C(dns_suffix) (list): List of domain suffixes, also known as DNS search path (default: C(domain) parameter).'
- ' - C(dns_suffix) (list): List of domain suffixes, also known as DNS search path (default: C(domain) parameter).
Also accepts space separated domain suffixes.'
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not advertise space-delimited lists as this is only for backward compatibility. We don't want to offer people this choice, only support people that did not have the choice before. I would even deprecate space-delimited lists over time.

@dagwieers
Copy link
Contributor

Over time we should use suboptions and argspec options.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 17, 2018
@dagwieers
Copy link
Contributor

Should we add/update tests ?

@Akasurde
Copy link
Member Author

@dagwieers VCSIM does not support customisation (yet) so I don't think so we can add/update any test.

@Akasurde
Copy link
Member Author

@dagwieers I removed the doc update. Please take a look.

dagwieers
dagwieers approved these changes Jul 17, 2018
View reviewed changes
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.

Looking good mostly. Only some notes on cosmetic changes for consistent documentation/examples.

lib/ansible/modules/cloud/vmware/vmware_guest.py Outdated
hostname: "{{ vcenter_server }}"
username: "{{ vcenter_user }}"
password: "{{ vcenter_password }}"
validate_certs: False
Copy link
Contributor

Choose a reason for hiding this comment

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

We standardized to using yes/no booleans in documentation (mostly because it is easier to read and reflects programming less).

lib/ansible/modules/cloud/vmware/vmware_guest.py Outdated
validate_certs: False
datacenter: "{{ datacenter }}"
state: present
folder: "/{{datacenter}}/vm"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to quote strings here.

lib/ansible/modules/cloud/vmware/vmware_guest.py Outdated
folder: "/{{datacenter}}/vm"
template: "{{ template }}"
name: "{{ vm_name }}"
cluster: "DC1_C1"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to quote strings here.

lib/ansible/modules/cloud/vmware/vmware_guest.py Outdated
name: "{{ vm_name }}"
cluster: "DC1_C1"
networks:
- name: "VM Network"
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to quote strings here.

@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 17, 2018
@ansibot ansibot added the community_review In order to be merged, this PR must follow the community review workflow. label Jul 17, 2018
@Akasurde
VMware: dns_suffix as a list of domain suffixes
d49d607 
This fix adds dns_suffix as a list of domain suffixes and
also updates documentation and example.

Fixes: ansible#42229

Signed-off-by: Abhijeet Kasurde <akasurde@redhat.com>
dagwieers
dagwieers approved these changes Jul 17, 2018
View reviewed changes
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.

LGTM

lib/ansible/modules/cloud/vmware/vmware_guest.py
- name: VM Network
ip: 192.168.10.11
netmask: 255.255.255.0
wait_for_ip_address: True
Copy link
Contributor

Choose a reason for hiding this comment

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

We standardized to using yes/no booleans in documentation (mostly because it is easier to read and reflects programming less).

@ansibot ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed community_review In order to be merged, this PR must follow the community review workflow. labels Jul 17, 2018
@ansibot ansibot merged commit db9c9e7 into ansible:devel Jul 17, 2018
@Akasurde Akasurde deleted the i42229 branch July 18, 2018 03:45
@Akasurde
Copy link
Member Author

@dagwieers Thanks for the reviews.

@dagwieers
Copy link
Contributor

@Akasurde You're welcome. I hadn't expected @ansibot to automerge. I still don't know when exactly automerge is activated and when not...

@Tomorrow9 Tomorrow9 mentioned this pull request Jul 17, 2019
Closed
@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

@dagwieers dagwieers dagwieers approved these changes

Assignees
No one assigned
Labels
affects_2.7 This issue/PR affects Ansible v2.7 automerge This PR was automatically merged by ansibot. bug This issue/PR relates to a bug. cloud module This issue/PR relates to a module. owner_pr This PR is made by the module's maintainer. shipit This PR is ready to be merged by Core 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.

vmware_guest doesn't properly format search in /etc/resolv.conf file
4 participants
@Akasurde @ansibot @dagwieers @samdoran