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

Remove embedded-capi feature #1029

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

anmazzotti
Copy link
Contributor

@anmazzotti anmazzotti commented Jan 22, 2025

What this PR does / why we need it:

This PR removes the embedded-capi feature, since it's now managed by the pre-install job and it is recommended to let the Turtles chart manage this setting.

The test suites have also been removed.

Also note that this will have the side effect of always creating the following resource:

apiVersion: management.cattle.io/v3
kind: Feature
metadata:
  name: embedded-cluster-api
  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/hook-weight": "1"
spec:
  value: false

Rancher does not seem to create it by default (as other Features are).
The Turtles chart will create it or override any existing, which is the desired behavior, however the chart will not clean this up.

The documentation already covers this: https://turtles.docs.rancher.com/turtles/v0.15/en/getting-started/uninstall_turtles.html

Test run: https://github.com/rancher/turtles/actions/runs/12903793122

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 #

Special notes for your reviewer:

Checklist:

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

Copy link
Member

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

we should add an e2e test for rancherInstalled=false

related issue #1025

salasberryfin
salasberryfin previously approved these changes Jan 22, 2025
Signed-off-by: Andrea Mazzotti <[email protected]>
@anmazzotti anmazzotti force-pushed the remove_embedded_capi_feature branch from f9ecd24 to 35e8033 Compare January 22, 2025 10:09
@furkatgofurov7
Copy link
Contributor

@anmazzotti
Copy link
Contributor Author

@furkatgofurov7 No that is good as it is and still required.
The chart does not take care of cleaning up the added Feature, so this is still a manual step the user has to take.

It can be improved by the helm chart by not overriding an already false value, and by deleting the Feature post uninstall, if it was part of the same helm release.

@furkatgofurov7 furkatgofurov7 mentioned this pull request Jan 22, 2025
4 tasks
@anmazzotti anmazzotti enabled auto-merge January 22, 2025 10:31
@anmazzotti anmazzotti merged commit 3d5dd5d into rancher:main Jan 22, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants