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

Jump to bottom

Updated vSphere cloud provider config for Kubernetes >= v1.9.2 and added resource pool deployment variable #2221

Merged
merged 4 commits into from Jan 31, 2018
Merged

Updated vSphere cloud provider config for Kubernetes >= v1.9.2 and added resource pool deployment variable #2221

merged 4 commits into from Jan 31, 2018

Conversation

Xuxe
Copy link
Contributor

@Xuxe Xuxe commented Jan 31, 2018

Hi all,

as discussed here:
kubernetes/kubernetes#58927
and documented here:
https://vmware.github.io/vsphere-storage-for-kubernetes/documentation/existing.html

the syntax for the vSphere cloud provider has been slightly changed for Kubernetes >= 1.9.2

I updated the vsphere-cloud-provider template and added a new deployment variable for resource pools.
Is it okay to make the decision inside the template? Or should we create seperated tasks for it?

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 31, 2018
@bradbeam
Copy link
Contributor

Overall changes lgtm, is there any way you can refactor it so the template is a bit more DRY?

@bradbeam bradbeam self-assigned this Jan 31, 2018
bradbeam
bradbeam reviewed Jan 31, 2018
View reviewed changes
docs/vsphere.md Outdated
@@ -34,10 +34,12 @@ Then, in the same file, you need to declare your vCenter credential following th
| vsphere_datastore | TRUE | string | | | Datastore name to use |
| vsphere_working_dir | TRUE | string | | | Working directory from the view "VMs and template" in the vCenter where VM are placed |
| vsphere_scsi_controller_type | TRUE | string | buslogic, pvscsi, parallel | pvscsi | SCSI controller name. Commonly "pvscsi". |
| vsphere_vm_uuid | FALSE | string | | | VM Instance UUID of virtual machine that host K8s master. Can be retrieved from instanceUuid property in VmConfigInfo, or as vc.uuid in VMX file or in `/sys/class/dmi/id/product_serial` |
| vsphere_vm_uuid | FALSE | string | | | VM Instance UUID of virtual machine that host K8s master. Can be retrieved from instanceUuid property in VmConfigInfo, or as vc.uuid in VMX file or in `/sys/class/dmi/id/product_serial` (only optional for Kubernetes <= 1.9.2) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the wording to Optional, only used for Kubernetes <= 1.9.2

@Xuxe Xuxe force-pushed the patch-vcp-v1.9.2 branch 5 times, most recently from 1ca0c01 to 9cdd221 Compare January 31, 2018 15:50
@Xuxe
Copy link
Contributor Author

Xuxe commented Jan 31, 2018

@bradbeam
The template should be okay now?
I have updated the docs also.

@bradbeam
Copy link
Contributor

ci check this

@bradbeam
Copy link
Contributor

@Xuxe just need to wait on ci and we should be good.

@rsmitty
Copy link
Contributor

rsmitty commented Jan 31, 2018

passed ci. merging.

@rsmitty rsmitty merged commit c0a3bcf into kubernetes-sigs:master Jan 31, 2018
@bradbeam bradbeam mentioned this pull request Mar 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bradbeam bradbeam bradbeam approved these changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Xuxe @k8s-ci-robot @bradbeam @rsmitty