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

Enable automated install of vsphere cpi/csi #103

Merged
merged 1 commit into from
Apr 12, 2024
Merged

Enable automated install of vsphere cpi/csi #103

merged 1 commit into from
Apr 12, 2024

Conversation

slickwarren
Copy link
Contributor

@slickwarren slickwarren commented Mar 14, 2024 •
edited
Loading

rancher/rancher#44787

enables the following option in the config:

provisioningInput:
  cloudProvider: "rancher-vsphere"

which will allow the user to use vsphere with either rke1 or rke2.

Other Improvements:

  • added an option to populate default values for a given chart
  • formatting updates
  • added DatastoreUrl for vsphere rke2 and rke1

@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch 3 times, most recently from b5ca42b to 16f0740 Compare March 15, 2024 17:52
@slickwarren slickwarren added the area/release-testing Related to Release Testing label Mar 20, 2024
@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch from 16f0740 to b5b2251 Compare March 20, 2024 21:15
@slickwarren slickwarren marked this pull request as ready for review March 21, 2024 16:10
@igomez06 igomez06 added automation-enhancement To indicate this is related to enhancements in code related to automation and removed area/release-testing Related to Release Testing labels Mar 21, 2024
Josh-Diamond
Josh-Diamond reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@Josh-Diamond Josh-Diamond left a comment

Choose a reason for hiding this comment

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

left one comment -- overall this looks great

extensions/provisioning/creates.go Outdated Show resolved Hide resolved
markusewalker
markusewalker approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@markusewalker markusewalker left a comment

Choose a reason for hiding this comment

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

Outside of Josh's comment, LGTM.

@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch from b5b2251 to b6b9172 Compare March 22, 2024 22:59
igomez06
igomez06 reviewed Mar 29, 2024
View reviewed changes
Copy link

@igomez06 igomez06 left a comment

Choose a reason for hiding this comment

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

overall good, some nits.

extensions/cloudcredentials/vsphere/create.go Show resolved Hide resolved
extensions/rke1/nodetemplates/vsphere/create.go Outdated
return resp, nil
}

func GetVsphereDatastoreUrl() string {
Copy link

Choose a reason for hiding this comment

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

missing comment here.

Copy link

Choose a reason for hiding this comment

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

should be GetVsphereDatastoreURL per camel case guidelines 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

extensions/rke1/nodetemplates/vsphere/create.go Show resolved Hide resolved
@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch from b6b9172 to 27e53e1 Compare March 29, 2024 16:08
igomez06
igomez06 reviewed Mar 29, 2024
View reviewed changes
Copy link

@igomez06 igomez06 left a comment

Choose a reason for hiding this comment

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

One last style nit, since we just had a meeting about it

@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch from 27e53e1 to 62d5a0e Compare March 29, 2024 18:40
igomez06
igomez06 previously approved these changes Mar 29, 2024
View reviewed changes
Copy link

@igomez06 igomez06 left a comment

Choose a reason for hiding this comment

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

LGTM

This was referenced Apr 1, 2024
Merged
Merged
caliskanugur
caliskanugur reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@caliskanugur caliskanugur left a comment

Choose a reason for hiding this comment

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

Looks good to me, I've got a few questions

clients/rancher/catalog/clusterrepo.go Outdated Show resolved Hide resolved
extensions/charts/vsphereoutoftree.go Show resolved Hide resolved
extensions/charts/vsphereoutoftree.go Show resolved Hide resolved
extensions/charts/vsphereoutoftree.go Show resolved Hide resolved
extensions/provisioning/creates.go Show resolved Hide resolved
@slickwarren slickwarren force-pushed the cwarren/vsphere-cpi-csi branch from 62d5a0e to a6c009a Compare April 5, 2024 16:26
caliskanugur
caliskanugur approved these changes Apr 8, 2024
View reviewed changes
Copy link
Member

@caliskanugur caliskanugur left a comment

Choose a reason for hiding this comment

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

Looks good to me

igomez06
igomez06 approved these changes Apr 12, 2024
View reviewed changes
Copy link

@igomez06 igomez06 left a comment

Choose a reason for hiding this comment

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

LGTM

@igomez06 igomez06 merged commit f816adc into rancher:main Apr 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markusewalker markusewalker markusewalker approved these changes

@caliskanugur caliskanugur caliskanugur approved these changes

@igomez06 igomez06 igomez06 approved these changes

@susesgartner susesgartner Awaiting requested review from susesgartner

@Josh-Diamond Josh-Diamond Awaiting requested review from Josh-Diamond

Assignees
No one assigned
Labels
automation-enhancement To indicate this is related to enhancements in code related to automation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@slickwarren @igomez06 @caliskanugur @Josh-Diamond @markusewalker