Fixing release-5 prefix bug for OCP 5.0#728
Fixing release-5 prefix bug for OCP 5.0#728openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
WalkthroughA new helper function Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/release-controller/sync_verify.go (1)
243-250: Prefer unexported helper for local-only usage
TrimPrefixesappears file-local. Making it unexported keeps package surface tighter.♻️ Proposed refactor
- stream := fmt.Sprintf("%s.0-0.%s%s", upgradeRelease.Candidate.Version, upgradeRelease.Candidate.Stream, TrimPrefixes(release.Config.To, "release-5", "release")) + stream := fmt.Sprintf("%s.0-0.%s%s", upgradeRelease.Candidate.Version, upgradeRelease.Candidate.Stream, trimPrefixes(release.Config.To, "release-5", "release"))-func TrimPrefixes(s string, prefixes ...string) string { +func trimPrefixes(s string, prefixes ...string) string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/release-controller/sync_verify.go` around lines 243 - 250, TrimPrefixes is only used locally so make it unexported by renaming it to trimPrefixes and update all local callers (e.g., places referencing TrimPrefixes in this file) to the new name; keep the implementation and signature identical (strings.CutPrefix loop) to preserve behavior and run tests to verify there are no remaining references to the exported name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/release-controller/sync_verify.go`:
- Line 223: The TrimPrefixes usage when building the stream string (see stream
:= fmt.Sprintf(...) using TrimPrefixes) can produce malformed suffixes by
naively cutting prefixes like "release-5" from values such as "release-50";
update TrimPrefixes (function TrimPrefixes) to perform boundary-aware trimming:
after a candidate prefix is removed only accept the trimmed result if it is
empty or its first character is '.' or '-' (otherwise keep the original input).
Replace the existing strings.CutPrefix-based behavior with this boundary check
so stream construction uses only safe suffixes.
---
Nitpick comments:
In `@cmd/release-controller/sync_verify.go`:
- Around line 243-250: TrimPrefixes is only used locally so make it unexported
by renaming it to trimPrefixes and update all local callers (e.g., places
referencing TrimPrefixes in this file) to the new name; keep the implementation
and signature identical (strings.CutPrefix loop) to preserve behavior and run
tests to verify there are no remaining references to the exported name.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
cmd/release-controller/sync_verify.go
| // create blank semver.Range | ||
| var constraint semver.Range | ||
| stream := fmt.Sprintf("%s.0-0.%s%s", upgradeRelease.Candidate.Version, upgradeRelease.Candidate.Stream, strings.TrimPrefix(release.Config.To, "release")) | ||
| stream := fmt.Sprintf("%s.0-0.%s%s", upgradeRelease.Candidate.Version, upgradeRelease.Candidate.Stream, TrimPrefixes(release.Config.To, "release-5", "release")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n cmd/release-controller/sync_verify.go | sed -n '215,255p'Repository: openshift/release-controller
Length of output: 1850
🏁 Script executed:
rg "TrimPrefixes" --type goRepository: openshift/release-controller
Length of output: 367
🏁 Script executed:
rg "Config\.To" --type go -A 2 -B 2 | head -100Repository: openshift/release-controller
Length of output: 678
🏁 Script executed:
rg "release-\d" --type go | head -50Repository: openshift/release-controller
Length of output: 1805
🏁 Script executed:
rg "type.*Config struct" --type go -A 30 | grep -A 30 "To.*string"Repository: openshift/release-controller
Length of output: 50384
🏁 Script executed:
rg "Config.*struct" --type go cmd/release-controller/ -A 20 | head -80Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
rg "\.To\s*=" --type go cmd/release-controller/ -B 5 -A 2 | head -80Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
fd -t f "\.go$" cmd/release-controller/ | xargs grep -l "type Config struct" | head -5Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
find cmd/release-controller -name "*.go" -exec grep -l "\.To\s" {} \; | head -5Repository: openshift/release-controller
Length of output: 109
🏁 Script executed:
rg "type.*Config\s+struct" cmd/release-controller/ -A 20Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
cat -n cmd/release-controller/sync_verify.go | sed -n '190,225p'Repository: openshift/release-controller
Length of output: 2402
🏁 Script executed:
rg "release\.Config\.To" cmd/release-controller/ -B 5 -A 5Repository: openshift/release-controller
Length of output: 1162
🏁 Script executed:
rg "Config struct" cmd/release-controller/ -A 15Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
rg "type Release struct" --type go -A 20Repository: openshift/release-controller
Length of output: 6184
🏁 Script executed:
rg "Config\s+struct" --type go cmd/release-controller/ -A 15Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
# Find where release parameter comes from in the function signature
rg "func.*resolveUpgradeRelease" --type go -A 2Repository: openshift/release-controller
Length of output: 468
🏁 Script executed:
grep -n "^type ReleaseConfig struct" pkg/release-controller/types.go -A 50Repository: openshift/release-controller
Length of output: 2863
🏁 Script executed:
python3 << 'EOF'
def trim_prefixes(s, *prefixes):
for p in prefixes:
if s.startswith(p):
return s[len(p):]
return s
# Test cases from the review and realistic scenarios
cases = [
"release-5.0",
"release-50",
"release-50.1",
"release-4.19",
"release-5",
"release-5-edge",
"release-500",
]
for c in cases:
result = trim_prefixes(c, "release-5", "release")
print(f"{c:20} -> {result:10} (after prefix trim)")
EOFRepository: openshift/release-controller
Length of output: 456
🏁 Script executed:
# Test the proposed fix
python3 << 'EOF'
def trim_prefixes_fixed(s, *prefixes):
for p in prefixes:
if s.startswith(p):
after = s[len(p):]
# Proposed boundary check
if after == "" or after.startswith(".") or after.startswith("-"):
return after
return s
cases = [
"release-5.0",
"release-50",
"release-50.1",
"release-4.19",
"release-5",
"release-5-edge",
"release-500",
]
print("Original vs Fixed:")
for c in cases:
orig = "release-5" if c.startswith("release-5") else "release" if c.startswith("release") else c
result_orig = c[len("release-5"):] if c.startswith("release-5") else c[len("release"):] if c.startswith("release") else c
result_fixed = trim_prefixes_fixed(c, "release-5", "release")
print(f"{c:20} -> orig: {result_orig:10} fixed: {result_fixed:10} {'✓ SAME' if result_orig == result_fixed else '✗ DIFF'}")
EOFRepository: openshift/release-controller
Length of output: 552
🏁 Script executed:
# Look for actual values of Config.To in release configs
find . -name "*.yaml" -o -name "*.json" | xargs grep -l "to:" 2>/dev/null | head -5Repository: openshift/release-controller
Length of output: 570
🏁 Script executed:
# Check if there are tests or examples showing expected Config.To values
rg "Config.*To" pkg/release-controller/ cmd/release-controller/ -B 3 -A 3Repository: openshift/release-controller
Length of output: 1344
🏁 Script executed:
# Look for documentation about Config.To field values
rg "To.*image stream" --type go -B 5 -A 5Repository: openshift/release-controller
Length of output: 7144
🏁 Script executed:
# Check actual release controller documentation or comments
rg "\"to\"" pkg/release-controller/types.go -B 5 -A 5Repository: openshift/release-controller
Length of output: 2092
🏁 Script executed:
# See if there are examples in the codebase
find . -path ./vendor -prune -o -name "*.md" -type f -exec grep -l "release-5\|release-4" {} \; 2>/dev/null | head -5Repository: openshift/release-controller
Length of output: 54
🏁 Script executed:
# Verify the concern is realistic - check if "release-5" prefix can cause partial matches
python3 << 'EOF'
import re
# Check strings.CutPrefix behavior for problematic cases
def cut_prefix(s, prefix):
if s.startswith(prefix):
return s[len(prefix):]
return None
test_cases = [
("release-5.0", "release-5"), # Intended match
("release-50.1", "release-5"), # Unintended match (partial numeric)
("release-500", "release-5"), # Unintended match (partial numeric)
("release-5", "release-5"), # Intended match (exact)
("release-5-edge", "release-5"), # Intended match (boundary marker)
]
print("Problematic cases where release-5 causes partial numeric matches:")
for s, prefix in test_cases:
result = cut_prefix(s, prefix)
if result is not None:
# Check if result looks like it was partially matched
if result and result[0].isdigit(): # Result starts with a digit
print(f" {s:20} → {result:10} (starts with digit - possible unintended match)")
else:
print(f" {s:20} → {result:10}")
EOFRepository: openshift/release-controller
Length of output: 413
Add boundary-aware prefix trimming to prevent silent malformation of stream names
The TrimPrefixes function at lines 243-250 performs raw text prefix matching via strings.CutPrefix, which can accidentally match partial version numbers. For example, if release.Config.To contains "release-50" or "release-50.1", the prefix "release-5" will match and produce "0" or "0.1" respectively, creating malformed stream names passed to line 223.
The proposed boundary check (allowing only results that are empty or start with "." or "-") prevents silent generation of invalid stream suffixes.
🔧 Proposed fix
func TrimPrefixes(s string, prefixes ...string) string {
- for _, prefix := range prefixes {
- if after, found := strings.CutPrefix(s, prefix); found {
- return after
- }
- }
- return s
+ for _, prefix := range prefixes {
+ if after, found := strings.CutPrefix(s, prefix); found {
+ // Avoid partial token matches like release-50.x matching release-5
+ if after == "" || strings.HasPrefix(after, ".") || strings.HasPrefix(after, "-") {
+ return after
+ }
+ }
+ }
+ return s
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/release-controller/sync_verify.go` at line 223, The TrimPrefixes usage
when building the stream string (see stream := fmt.Sprintf(...) using
TrimPrefixes) can produce malformed suffixes by naively cutting prefixes like
"release-5" from values such as "release-50"; update TrimPrefixes (function
TrimPrefixes) to perform boundary-aware trimming: after a candidate prefix is
removed only accept the trimmed result if it is empty or its first character is
'.' or '-' (otherwise keep the original input). Replace the existing
strings.CutPrefix-based behavior with this boundary check so stream construction
uses only safe suffixes.
|
@bradmwilliams: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel, bradmwilliams The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
Summary by CodeRabbit