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

🌱 Add option to skip rancher pre-requisites #705

Merged

Conversation

Danil-Grigorev
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev commented Aug 30, 2024

What this PR does / why we need it:
This change should allow to install turtles in development scenario without dependency on rancher, for the expense of added requirement to patch the embedded-cluster-api feature gate manually.

This can be done by specifying --set rancherTurtles.rancherInstalled=false flag.

Deletion will need to be performed according to https://turtles.docs.rancher.com/getting-started/uninstall_turtles

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #627

Special notes for your reviewer:

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
@Danil-Grigorev Danil-Grigorev requested a review from a team as a code owner August 30, 2024 07:05
@Danil-Grigorev Danil-Grigorev changed the title Add option to skip rancher pre-requisites 🌱 Add option to skip rancher pre-requisites Aug 30, 2024
@hardys
Copy link
Contributor

hardys commented Aug 30, 2024

Thanks for working on this @Danil-Grigorev - the change looks good to me, although it's now clear we could potentially use the existing features.embedded-capi API to achieve the same thing, unless we expect any other dependencies on Rancher in the pre-install phase.

Note for other reviewers the use-case behind #627 is we want to deploy Rancher and Rancher Turtles via the RKE2 helm controller - in this situation ordering of the helm installs cannot be controlled, so you end up reliant on retries, so we must ensure the turtles chart can either install, or fail and be uninstalled, in the case where Rancher is not yet installed.

A related issue is in the pre-delete hook we assume the capiproviders CRD exists, and the hook fails otherwise - IMO for a robust fix we should consider making the pre-delete hook fail gracefully in this scenario (e.g check if the crd exists, and exit without any error if it's missing), but that could be considered as a follow-up perhaps.

salasberryfin
salasberryfin previously approved these changes Sep 2, 2024
Copy link
Contributor

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

@Danil-Grigorev can we also update the turtles docs regarding this, since current docs have rancher listed as pre-requisite and has a dedicated chapter for installing it before Turtles? I believe this provides a fix for the specific use case edge team has, but I suggest we need to be super clear on what are the consequences if this option is enabled.

furkatgofurov7
furkatgofurov7 previously approved these changes Sep 2, 2024
Signed-off-by: Danil-Grigorev <danil.grigorev@suse.com>
@Danil-Grigorev Danil-Grigorev merged commit 911323f into rancher:main Sep 2, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

issue during installation when rancher is not ready
5 participants