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

🌱 chore: cover uninstall in e2e tests #474

Merged
merged 2 commits into from
May 2, 2024

Conversation

salasberryfin
Copy link
Contributor

@salasberryfin salasberryfin commented Apr 9, 2024

What this PR does / why we need it:

After running the import-gitops test suite, we verify that helm uninstall works on Turtles. This also upgrades Helm to v3.14.0, as v3.8.1 was failing to detect the --cascade flag.

Which issue(s) this PR fixes:
Fixes #410

Special notes for your reviewer:

The uninstall of Turtles may fail unexpectedly due to a problem with the CAPI Operator Helm chart and CRDs being deleted in such a way that doesn't guarantee the order and may end up in a deadlock caused by a finalizer that is never removed.

Note: perhaps we should wait to have this sorted out before merging this change and having the uninstall step as part of the E2E tests.

Checklist:

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

@salasberryfin salasberryfin added area/testing Indicates an issue related to test kind/chore labels Apr 9, 2024
@salasberryfin salasberryfin requested a review from a team as a code owner April 9, 2024 14:10
@salasberryfin salasberryfin changed the title E2e cover uninstall 🌱 chore: cover uninstall in e2e tests Apr 9, 2024
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.

PR looks good but I have same concerns as @salasberryfin about merging it before solving uninstall issue upstream in CAPI Operator.

@furkatgofurov7
Copy link
Contributor

PR looks good but I have same concerns as @salasberryfin about merging it before solving uninstall issue upstream in CAPI Operator.

+1

@salasberryfin
Copy link
Contributor Author

Thanks for the reviews. Since we agree on waiting for the issue to be solved before merging this, I moved the original ticket to blocked.

Danil-Grigorev
Danil-Grigorev previously approved these changes Apr 30, 2024
Copy link
Contributor

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

This LGTM now, can you please rebase @salasberryfin?

richardcase
richardcase previously approved these changes May 1, 2024
Copy link
Contributor

@richardcase richardcase left a comment

Choose a reason for hiding this comment

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

This also LGTM from my side. After the rebase lets merge.

@Danil-Grigorev
Copy link
Contributor

I’ll take care of rebase as Carlos is on PTO.

Signed-off-by: Carlos Salas <carlos.salas@suse.com>
Signed-off-by: Carlos Salas <carlos.salas@suse.com>
@alexander-demicev alexander-demicev merged commit 79f15fa into rancher:main May 2, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Indicates an issue related to test kind/chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[e2e] Create a test to cover uninstall
5 participants