fix: align kured_version logic with csi/calico pattern#2032
Conversation
The kured_version assignment used a different pattern than csi_version and calico_version, checking `var.kured_version != null` instead of `length(data.github_release.kured) == 0`. This caused issues when the data source was created but terraform's evaluation order led to accessing the data source before it resolved. Use the same `length()` pattern that works reliably for csi and calico: - length == 0 means var was provided (data source not created) - length == 1 means var was null (use data source value) Fixes #2017
Summary of ChangesHello @mysticaltech, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the version assignment logic for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a potential null value issue with kured_version by aligning its assignment logic with the more robust pattern used for csi_version and calico_version. The change is well-explained and fixes the described problem. My only suggestion is to also apply this fix to ccm_version for consistency and to prevent similar issues, as it currently uses the old, potentially problematic pattern.
| ccm_version = var.hetzner_ccm_version != null ? var.hetzner_ccm_version : data.github_release.hetzner_ccm[0].release_tag | ||
| csi_version = length(data.github_release.hetzner_csi) == 0 ? var.hetzner_csi_version : data.github_release.hetzner_csi[0].release_tag | ||
| kured_version = var.kured_version != null ? var.kured_version : data.github_release.kured[0].release_tag | ||
| kured_version = length(data.github_release.kured) == 0 ? var.kured_version : data.github_release.kured[0].release_tag |
There was a problem hiding this comment.
This change correctly aligns the logic for kured_version with the safer pattern used elsewhere. However, the same logic is used for ccm_version on line 13, which is not updated. To ensure consistency and prevent potential Terraform errors, ccm_version should also be updated to use the length() check.
For example:
ccm_version = length(data.github_release.hetzner_ccm) == 0 ? var.hetzner_ccm_version : data.github_release.hetzner_ccm[0].release_tagThere was a problem hiding this comment.
Pull request overview
This PR updates the logic for determining kured_version so that it follows the same pattern as csi_version and calico_version, preventing local.kured_version from becoming null when kured_version is omitted. The goal is to ensure the semver comparison for kured_yaml_suffix works reliably with or without an explicitly set kured_version.
Changes:
- Updated
locals.tfto derivekured_versionfromdata.github_release.kuredwhen the data source is present and fall back tovar.kured_versionwhen it is not. - Aligned the
kured_versionselection logic with the existingcsi_versionandcalico_versionlength-based pattern to avoid indexing into an empty data source and to keep behavior consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
kured_version is nullerror whenkured_versionis omitted from kube.tfkured_versionassignment with the established pattern used bycsi_versionandcalico_versionProblem
When
kured_versionwas omitted, the semver comparison on line 19 failed:Solution
Changed from:
To:
This matches the pattern used for
csi_versionandcalico_version, which checks the data source existence directly vialength()rather than checking the variable.Test plan
terraform fmtpassesterraform validatepassesterraform planworks withkured_versionomitted (uses GitHub latest)terraform planworks with explicitkured_version = "1.20.0"Fixes #2017