Skip to content

vSphere UPI - Use a folder for virtual machines#1470

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jcpowermac:vsphere_use_folder
Mar 29, 2019
Merged

vSphere UPI - Use a folder for virtual machines#1470
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
jcpowermac:vsphere_use_folder

Conversation

@jcpowermac
Copy link
Copy Markdown
Contributor

@jcpowermac jcpowermac commented Mar 26, 2019 •
edited
Loading

  • Creates a vm folder
  • Virtual Machines are deployed into the folder created

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2019
@sdodson sdodson added this to the 4.1 beta 4 milestone Mar 27, 2019
@jcpowermac jcpowermac force-pushed the vsphere_use_folder branch 2 times, most recently from 9f91ae3 to 852561a Compare March 28, 2019 12:42
@jcpowermac jcpowermac changed the title [WIP] vSphere UPI - Use a folder for virtual machines vSphere UPI - Use a folder for virtual machines Mar 28, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 28, 2019
@jcpowermac
Copy link
Copy Markdown
Contributor Author

jcpowermac commented Mar 28, 2019

cc @staebler

staebler
staebler reviewed Mar 28, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

Changes look good. I have just a couple nits.

Did using the path from the vsphere_folder resource set up the dependency so that the folder is not deleted until the vms are deleted? Or do we still have the deficiency where terraform attempts to delete the folder too early?

Comment thread upi/vsphere/folder/variables.tf Outdated
Copy link
Copy Markdown
Contributor

@staebler staebler Mar 28, 2019

Choose a reason for hiding this comment

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

This variable is not used.

Comment thread upi/vsphere/machine/variables.tf Outdated
Copy link
Copy Markdown
Contributor

@staebler staebler Mar 28, 2019

Choose a reason for hiding this comment

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

I'm not fond of folder_id as the name of this variable. It is not an ID. It is the name of the folder. My preference would be for this to just be folder. I would be fine with folder_name or folder_path, too.

@staebler
Copy link
Copy Markdown
Contributor

staebler commented Mar 28, 2019

Can you change the commit message to start with "upi/vsphere" and remove the [WIP] from the commit message?

@staebler staebler mentioned this pull request Mar 28, 2019
Closed
@jcpowermac
vSphere UPI - Use a folder for virtual machines
c40969a 
- Creates a vm folder
- Virtual Machines are deployed into the folder created
@jcpowermac
Copy link
Copy Markdown
Contributor Author

jcpowermac commented Mar 29, 2019

Changes look good. I have just a couple nits.

Did using the path from the vsphere_folder resource set up the dependency so that the folder is not deleted until the vms are deleted? Or do we still have the deficiency where terraform attempts to delete the folder too early?

I tested a number of times to make sure that the folder was deleted after the vms. I wanted to make sure that worked before we went ahead with this PR.

@staebler
Copy link
Copy Markdown
Contributor

staebler commented Mar 29, 2019

Changes look good. I have just a couple nits.
Did using the path from the vsphere_folder resource set up the dependency so that the folder is not deleted until the vms are deleted? Or do we still have the deficiency where terraform attempts to delete the folder too early?

I tested a number of times to make sure that the folder was deleted after the vms. I wanted to make sure that worked before we went ahead with this PR.

Great.I have used it a couple times too, with success.

staebler
staebler approved these changes Mar 29, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@staebler staebler left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 29, 2019
@openshift-ci-robot
Copy link
Copy Markdown
Contributor

openshift-ci-robot commented Mar 29, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcpowermac, staebler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2019
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Mar 29, 2019

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c7c59c8 into openshift:master Mar 29, 2019
@jcpowermac jcpowermac deleted the vsphere_use_folder branch June 27, 2019 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaronlevy aaronlevy Awaiting requested review from aaronlevy

@wking wking Awaiting requested review from wking

1 more reviewer

@staebler staebler staebler approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@staebler staebler

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

4.1 beta 4

Development

Successfully merging this pull request may close these issues.

6 participants

@jcpowermac @staebler @openshift-ci-robot @openshift-bot @sdodson @openshift-merge-robot