-
Notifications
You must be signed in to change notification settings - Fork 19
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
fix: use pre-install helm hook to prepare rancher for turtles #192
fix: use pre-install helm hook to prepare rancher for turtles #192
Conversation
We may need to install Rancher before Rancher Turtles in the E2E environment for the |
cd29889
to
cd5b194
Compare
E2e tests are passing with this change, so failure here could be ignored. |
Thanks @Danil-Grigorev. Should we consider this PR superseded by yours #190 and close it? |
Let's merge this first and then #190 and keep them as independent PRs. |
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.
In my head i was thinking of something actually in the Rancher Turtles binary but this is an interesting approach.
I added flags for disabling embedded capi and deleting rancher webhooks: rancherTurtles:
...
features:
disable-embedded-capi:
enabled: true
cleanup-rancher-webhook:
enabled: true
image: rancher/kubectl Also, changed the deleting webhook |
cd5b194
to
3512d41
Compare
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.
Looks good, thanks for adding the flags in the values.
Although there is a chart linrt failure.
d719cd3
to
e888cce
Compare
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.
Some minor suggestions, but LGTM as is.
|
||
- name: Install Rancher | ||
run: helm install rancher rancher-stable/rancher --namespace cattle-system --create-namespace --set bootstrapPassword=rancheradmin --set replicas=1 --set hostname="e2e.dev.rancher" --set 'extraEnv[0].name=CATTLE_FEATURES' --set global.cattle.psp.enabled=false --version ${{ env.RANCHER_VERSION }} --wait | ||
|
||
- name: Run chart-testing (install) | ||
run: helm install rancher-turtles out/charts/rancher-turtles/ -n rancher-turtles-system --create-namespace --wait --set cluster-api-operator.cluster-api.enabled=false --set cluster-api-operator.enabled=false |
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.
run: helm install rancher-turtles out/charts/rancher-turtles/ -n rancher-turtles-system --create-namespace --wait --set cluster-api-operator.cluster-api.enabled=false --set cluster-api-operator.enabled=false | |
run: helm install rancher-turtles out/charts/rancher-turtles/ -n rancher-turtles-system --create-namespace --wait --set cluster-api-operator.cluster-api.enabled=false --set cluster-api-operator.enabled=false --set features. disable-embedded-capi.enabled=false --set features.cleanup-rancher-webhook.enabled=false |
As this workflow represents the default flow of installing rancher-turtles in a fresh cluster, we might not need to bring rancher with it. Just making sure the chart is installable.
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.
I understand your point but I would say, considering these flags are enable by default, we should test them as well to validate the chart.
- name: Add cert-manager chart repo | ||
run: helm repo add jetstack https://charts.jetstack.io | ||
|
||
- name: Add rancher chart repo | ||
run: helm repo add rancher-stable https://releases.rancher.com/server-charts/stable | ||
|
||
- name: Install cert-manager | ||
run: helm install cert-manager jetstack/cert-manager --namespace cert-manager --create-namespace --version ${{ env.CERT_MANAGER_VERSION }} --set installCRDs=true --wait | ||
|
||
- name: Install Rancher | ||
run: helm install rancher rancher-stable/rancher --namespace cattle-system --create-namespace --set bootstrapPassword=rancheradmin --set replicas=1 --set hostname="e2e.dev.rancher" --set 'extraEnv[0].name=CATTLE_FEATURES' --set global.cattle.psp.enabled=false --version ${{ env.RANCHER_VERSION }} --wait | ||
|
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.
- name: Add cert-manager chart repo | |
run: helm repo add jetstack https://charts.jetstack.io | |
- name: Add rancher chart repo | |
run: helm repo add rancher-stable https://releases.rancher.com/server-charts/stable | |
- name: Install cert-manager | |
run: helm install cert-manager jetstack/cert-manager --namespace cert-manager --create-namespace --version ${{ env.CERT_MANAGER_VERSION }} --set installCRDs=true --wait | |
- name: Install Rancher | |
run: helm install rancher rancher-stable/rancher --namespace cattle-system --create-namespace --set bootstrapPassword=rancheradmin --set replicas=1 --set hostname="e2e.dev.rancher" --set 'extraEnv[0].name=CATTLE_FEATURES' --set global.cattle.psp.enabled=false --version ${{ env.RANCHER_VERSION }} --wait |
In addition to https://github.com/rancher-sandbox/rancher-turtles/pull/192/files#r1348559835
Signed-off-by: Carlos Salas <[email protected]>
e888cce
to
47f5148
Compare
What this PR does / why we need it:
Currently, installing Rancher Turtles in an existing Rancher Manager after disabling embedded CAPI, results in validating/mutating webhooks being left behind after the feature is disabled. If you try and apply a CAPI manifest you will get an error:
To avoid having to manually delete these webhooks and disable the feature flag, this PR adds a
pre-install
Helm hook that:This is not the fanciest of solutions but it allows to install Rancher Turtles just using
helm install
in an existing Rancher Manager. We can later change Rancher and remove this hook if we find a more robust option.Which issue(s) this PR fixes:
Fixes #185
Special notes for your reviewer:
When this gets merged, we'll need to update the docs accordingly.
Checklist: