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(helm): handle missing (null) values in version check #5307

Merged
merged 4 commits into from Oct 27, 2023
Merged

Conversation

vvagaytsev
Copy link
Collaborator

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #5303

Special notes for your reviewer:

@vvagaytsev vvagaytsev requested review from twelvemo and a team October 27, 2023 09:10
@vvagaytsev
Copy link
Collaborator Author

The ingress controller-specific issue reported in the target ticket will be resolved in #5136. However, the getReleaseStatus function should still be able to handle possible null values returned from the helm command.

Copy link
Collaborator

@twelvemo twelvemo left a comment

Choose a reason for hiding this comment

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

Nice! Had one suggesting around the error message.

core/src/plugins/kubernetes/helm/status.ts Outdated Show resolved Hide resolved
Co-authored-by: Anna Mager <78752267+twelvemo@users.noreply.github.com>
Copy link
Collaborator

@twelvemo twelvemo left a comment

Choose a reason for hiding this comment

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

Took another look and had another question.

core/src/plugins/kubernetes/helm/status.ts Outdated Show resolved Hide resolved
state = "outdated"
} else {
log.verbose(`No helm values returned for release ${releaseName}. Is this release managed outside of garden?`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we log this when the values are null?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, right :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed this in 8605647.

Copy link
Collaborator

@twelvemo twelvemo left a comment

Choose a reason for hiding this comment

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

🚀

@vvagaytsev vvagaytsev added this pull request to the merge queue Oct 27, 2023
Merged via the queue into main with commit 31f4420 Oct 27, 2023
44 checks passed
@vvagaytsev vvagaytsev deleted the fix/5303 branch October 27, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash: Cannot read properties of null (reading '.garden')
2 participants