-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Add VMware vSphere provider #3419
Conversation
eb7f82c
to
231d787
Compare
👍 |
👍 👏 What is the plan to get this added into a tag / release? Who wants to open an enhancement request so that we can start brainstorming on next steps? Or I am happy to.... @tkak どうもありがとうございました .... hope google got thank-you very much in japanese correct :) |
@chrislovecnm Your Japanese comment is correct! Thank you :) |
This is FANTASTIC! Looking forward ot having this in the core build! Thank you guys. Much obliged @tkak |
Optional: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
ForceNew: true, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeList
is for attributes where the order of the values is significant. If order does not matter, TypeSet
is what you want. I'm not familiar with the upstream APIs so this may be correct, but I wanted to let you know so you can double check this for each of your TypeList
attributes.
Alright, completed initial code review! This is generally looking really solid. 👍 |
@phinze thanks! Does @tkak now need to make changes? Or do you want to merge the pull and have an issue to apply the changes. Not familiar with your code review process, read through https://github.com/hashicorp/terraform/blob/master/CONTRIBUTING.md ... So would appreciate some guidance 😄 |
@chrislovecnm Generally we give original authors a chance to iterate on their initial PR before we land it. Though if @tkak would like to hand off the improvements to another willing community member that would be OK too. Up to you, @tkak 😀 |
@tkak 先輩 if you could make the changes that would be awesome! |
…n d.Set for complex types
@phinze Thanks for your review! I tried to fix some code that were pointed out. And, I want to use @chrislovecnm My target is VMware vSphere / vCenter 5.5. I mainly use this. Version 6 is not yet introduced in our environment. And govmomi is tested against ESXi and vCenter 5.5. |
Thanks @tkak! I'm going to merge this as-is and I'll get the provider docs in separately. That will also allow the community to get started on more features / improvements as well. 👍 |
If folks watching this thread are willing to give me a review on #3483 - that gets the structure set up for docs. 👍 |
) | ||
|
||
const ( | ||
defaultInsecureFlag = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a security hole.
SSL certificate validation must be switched on by default, and has to be disabled by a user explicitly.
Such answer was OK while you maintained a standalone 3rd-party plugin. Now when the code has been merged into the product, things are different. HashiCorp people have no experience in VMware technologies and tools, so you are the maintainer, and you are responsible of having this feature working well. |
@mkuzmin Thanks for the review! Your inline comments make sense - going to fan those out into individual issues so we can address them. This is just a first step - @tkak got us started, and now the community can collaborate to improve and extend from here. Since there has been so much community demand for vSphere support in core, we felt that it was better to get a pre-release version merged so we can have more people help out with its development. To help make sure users are aware of this status, I added notes about "initial support" in both the changelog and the docs.
Alas, this is still a manual process. I'm investigating the possibility of gaining access to a vSphere environment for us to run acceptance tests on. I'll link the new issues I create back to this thread for continuity. 👍 |
Do you think it's better to move all further conversations into new issues, and stop this thread? |
Yep I was thinking we can close this thread and address each of those concerns as individual issue threads, since they're not tightly related. I'll get these made now so you can see what I mean. |
@mkuzmin Thank you for the review and the advice.
I think you are right. I will try to prepare a test environment in my company to support VMware vSphere 6. |
This is awesome. I am sorry I have not been following the vSphere provider thread for a while, but I am back. I will work on providing a patch to the merged vSphere provider for the pieces I was working on. |
I note the TODO documentation and comment "I'll get the provider docs in separately. " Has documentation been updated? I ask because there are details in the original project (such as network adapter notes) that I didn't see on the terraform docs provider section on the website. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This adds VMware vSphere provider related to #102. This is a direct port from my Terraform plugin for VMware vSphere, terraform-provider-vsphere. Please refer the README for more detail usage.
Example
TODO: