fix(clustertest): resolve version from git when env var is empty#322
Open
tsivaprasad wants to merge 5 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughRefactored image tag initialization and version resolution logic. Moved Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
clustertest/main_test.go (1)
28-37: Consider failing early when version cannot be resolved.If the environment variable is unset and
git describealso fails (e.g., no matching tags, shallow clone, or git unavailable),resolveVersion()returns an empty string. This will produce an invalid image tag like127.0.0.1:5000/control-plane:and the build will fail with the same cryptic error the PR aims to fix.A clear early failure would improve debuggability:
Proposed improvement
func resolveVersion() string { if v := os.Getenv("CONTROL_PLANE_VERSION"); v != "" { return v } out, err := exec.Command("git", "-C", "..", "describe", "--tags", "--abbrev=0", "--match", "v*").Output() if err != nil { - return "" + log.Printf("warning: unable to resolve CONTROL_PLANE_VERSION from git: %v", err) + return "" } return strings.TrimSpace(string(out)) }Or in
TestMain, validate before use:version := resolveVersion() if version == "" { log.Fatal("CONTROL_PLANE_VERSION is empty and git describe failed; set the env var or ensure tags exist") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clustertest/main_test.go` around lines 28 - 37, resolveVersion() can return an empty string (when env var unset and `git describe` fails) which yields an invalid image tag; update the test bootstrap to validate the version early: in TestMain (or wherever resolveVersion() is consumed) call version := resolveVersion() and if version == "" call log.Fatalf (or t.Fatalf) with a clear message telling the user to set CONTROL_PLANE_VERSION or ensure tags/git are available, so the process fails immediately with a helpful error instead of producing an invalid image tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@clustertest/main_test.go`:
- Around line 28-37: resolveVersion() can return an empty string (when env var
unset and `git describe` fails) which yields an invalid image tag; update the
test bootstrap to validate the version early: in TestMain (or wherever
resolveVersion() is consumed) call version := resolveVersion() and if version ==
"" call log.Fatalf (or t.Fatalf) with a clear message telling the user to set
CONTROL_PLANE_VERSION or ensure tags/git are available, so the process fails
immediately with a helpful error instead of producing an invalid image tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 776d6125-4c4f-4eae-beda-251e57696594
📒 Files selected for processing (2)
clustertest/main_test.goclustertest/utils_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clustertest/main_test.go`:
- Around line 29-31: When reading CONTROL_PLANE_VERSION via
os.Getenv("CONTROL_PLANE_VERSION") (variable v), trim whitespace (e.g.,
strings.TrimSpace) and treat the value as unset if the trimmed string is empty;
update the branch that currently returns v to instead trim v, check if the
trimmed value is non-empty, and only then return it so whitespace-only env
values won't be treated as a valid image tag.
- Around line 40-44: The code currently calls resolveVersion() and fatals if
empty before flags are parsed, preventing runs that supply -image-tag from
working; change the logic so resolveVersion() is only required when no image-tag
flag is provided: parse flags first (or check the image-tag flag), and if
imageTag flag is empty then call resolveVersion() and fatal only if it returns
empty; set defaultImageTag using the resolved version when needed (referencing
resolveVersion(), defaultImageTag, and the image-tag flag/variable).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1007faf-b9ab-4e70-b7b4-d4fee0ee852c
📒 Files selected for processing (1)
clustertest/main_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
clustertest/main_test.go (1)
35-37:⚠️ Potential issue | 🟡 MinorTreat whitespace-only
CONTROL_PLANE_VERSIONas unset.At Line 35, the env var is used without trimming. Values like
" "bypass emptiness intent and can still yield invalid image tags.Suggested fix
import ( "flag" "log" "os" + "strings" "testing" @@ - version := os.Getenv("CONTROL_PLANE_VERSION") + version := strings.TrimSpace(os.Getenv("CONTROL_PLANE_VERSION")) if version == "" { log.Fatal("CONTROL_PLANE_VERSION is not set; ensure common.mk version resolution succeeded or pass -image-tag explicitly") }#!/bin/bash rg -n -C2 'CONTROL_PLANE_VERSION|TrimSpace' clustertest/main_test.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clustertest/main_test.go` around lines 35 - 37, The code reads version := os.Getenv("CONTROL_PLANE_VERSION") and treats any non-empty string (including whitespace-only) as valid; change to trim whitespace and treat whitespace-only as unset by using strings.TrimSpace on the os.Getenv result before the empty check (i.e., replace direct use of os.Getenv("CONTROL_PLANE_VERSION") with a trimmed value) so the subsequent if version == "" correctly catches values like " ". Ensure you import the strings package if not already used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@common.mk`:
- Around line 12-18: The CONTROL_PLANE_VERSION assignment can evaluate to empty
when both git describe (matching $(CHANGIE_LATEST)) and $(CHANGIE_LATEST) are
empty; update the deferred-evaluation fallback so it tries three tiers: first
git describe with the pattern $(CHANGIE_LATEST)*, then git describe with a
generic tag match like v* (to pick the latest semver tag), and finally a
hardcoded safe default such as v0.0.0-dev; implement this inside the same
deferred-simple-variable-expansion expression that sets CONTROL_PLANE_VERSION so
the shell pipeline attempts the first git describe, if that fails attempts a
second git describe --match 'v*', and if that fails echoes 'v0.0.0-dev',
ensuring CONTROL_PLANE_VERSION is never empty.
---
Duplicate comments:
In `@clustertest/main_test.go`:
- Around line 35-37: The code reads version :=
os.Getenv("CONTROL_PLANE_VERSION") and treats any non-empty string (including
whitespace-only) as valid; change to trim whitespace and treat whitespace-only
as unset by using strings.TrimSpace on the os.Getenv result before the empty
check (i.e., replace direct use of os.Getenv("CONTROL_PLANE_VERSION") with a
trimmed value) so the subsequent if version == "" correctly catches values like
" ". Ensure you import the strings package if not already used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b4e2c67-dd85-435c-9973-b019138f87de
📒 Files selected for processing (2)
clustertest/main_test.gocommon.mk
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a CI failure where cluster tests crash immediately on image build
because
CONTROL_PLANE_VERSIONis empty whendocker buildx bakeruns,producing an invalid image tag (
repo:).Changes
resolveVersion()inmain_test.gothat readsCONTROL_PLANE_VERSIONfrom the environment and falls back to
git describe --tags --abbrev=0 --match 'v*'when the var is absent or emptybuildImage()inutils_test.goto strip any inheritedCONTROL_PLANE_VERSION=(including empty) from the child processenvironment and replace it with the resolved value, preventing an
empty string from blocking
make's?=defaultTesting
previously failing with
invalid tag "172.17.0.1:5000/control-plane:": invalid reference format)Checklist