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

fix(upgrade): fix errors during chart version upgrade #138

Merged
merged 1 commit into from
May 21, 2024

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented May 13, 2024

Related to #124

What's changed

  • Wrapped new configurations in () for nil-checks. See here.

    • However, this is just for safe guarding, not solving the upgrade failures. See below.
  • Create a wrapper that forces helm to use --reset-then-reuse-values. This is available only with helm 3.14+.

    • The user cannot use helm upgrade --reuse-values as it would cause values such as (.Values.storage).image.repository to be empty. This is an invalid image repository.
    • Helm 3.14+ has a new feature with --reset-then-reuse-values: feat(helm): Add --reset-then-reuse-values flag to 'helm upgrade' helm/helm#9653 that fits our upgrade path more.
    • However, chart-testing has not been updated for accommodate that so this is a temporary fix.
  • Removed unused test value files (minimal mode is removed).

  • Upgraded related github actions.

What's motivation

The test is failing because chart-testing ct is also testing upgrade path with --reuse-values. This causes nil pointer error as old values do not have newly defined fields in 3.0.

Sample run

Sample PR: https://github.com/tthvo/cryostat-helm/actions/runs/9061712431/job/24894069674?pr=7

@tthvo tthvo requested a review from a team May 13, 2024 11:10
@tthvo tthvo mentioned this pull request May 13, 2024
10 tasks
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Seems like this makes sense.

@andrewazores andrewazores requested a review from ebaron May 13, 2024 13:42
@andrewazores
Copy link
Member

@ebaron WDYT?

Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Thanks for doing this work @tthvo. It's a shame the tester doesn't support this case, since we're not planning on upgrades from 2.x to 3.0 working out of the box in all scenarios.

@ebaron ebaron merged commit 9a36228 into cryostatio:cryostat3 May 21, 2024
2 checks passed
@tthvo
Copy link
Member Author

tthvo commented May 21, 2024

I am hoping chart-testing would add a patch to support each upgrade strategy after helm/chart-testing#525. If not, I can try in the next couple days as many might run into the same issues as us :D

@tthvo tthvo deleted the chart-upgrade branch May 25, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants