Use prefix matching for dependent bug target version validation - #190
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughDependent and direct target-version validation now use project-specific matching. DFBUGS requires exact configured-version equality. Other projects compare normalized major/minor versions and support an optional ChangesTarget-version validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/jira-lifecycle-plugin/server.go`:
- Around line 1688-1690: Update validateTargetVersion and its version-comparison
flow to remove the optional openshift- prefix from non-DFBUGS target versions
before applying truncateVersionToMajorMinor, preserving matches such as
openshift-4.22.z with configured 4.22.0. Add a regression test covering this
alias for a non-DFBUGS project.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0b14f18-83b3-4524-bf5a-86ecfdb93e51
📒 Files selected for processing (2)
cmd/jira-lifecycle-plugin/server.gocmd/jira-lifecycle-plugin/server_test.go
| } | ||
| } | ||
|
|
||
| func strPtr(s string) *string { |
There was a problem hiding this comment.
Can we use the k8s library function for this instead of making our own here?
There was a problem hiding this comment.
Good question — I checked the vendored dependencies and there isn't a suitable k8s library function for this:
k8s.io/apimachinery/pkg/versionhandles Kubernetes API group versions (v1,v2beta1), not product semver versions like4.22.0.- There's no
pkg/util/versionpackage vendored in this project. - No other semver/version parsing libraries are in the dependency tree.
Since all we need is to split on . and take the first two segments (a 5-line function), a custom helper seemed more appropriate than pulling in a new dependency. Happy to revisit if you know of a specific library that would fit.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
Yes! Added in the latest push (commit ff61559):
TestTruncateVersionToMajorMinor— 8 test cases covering: normal version (4.22.0→4.22), z-stream,openshift-prefix variants (3 cases), two-segment, single segment, and empty string.TestDependentTargetVersionMatch— 9 test cases covering: exact match, prefix match, no match,openshift-prefix (both directions), DFBUGS exact matching, multiple configured versions, and nil field edge cases.- Also added an integration test in
TestValidateBugforopenshift-4.22.zmatching configured4.22.0through the full validation flow.
AI-generated. Review for accuracy.
ff61559 to
393e58f
Compare
|
/cc @AlexNPavel |
| // truncateVersionToMajorMinor truncates a version string like "4.22.0" to "4.22". | ||
| // It strips the "openshift-" prefix if present (e.g. "openshift-4.22.z" becomes "4.22"). | ||
| // If the version has fewer than 2 dot-separated segments, it is returned as-is | ||
| // (with the prefix still stripped). | ||
| func truncateVersionToMajorMinor(version string) string { | ||
| version = strings.TrimPrefix(version, "openshift-") | ||
| pieces := strings.Split(version, ".") | ||
| if len(pieces) >= 2 { | ||
| return fmt.Sprintf("%s.%s", pieces[0], pieces[1]) | ||
| } | ||
| return version | ||
| } | ||
|
|
||
| // dependentTargetVersionMatch checks whether the dependent bug's target version | ||
| // matches any of the configured valid target versions. For DFBUGS projects, an | ||
| // exact match is required (consistent with validateTargetVersion). For all other | ||
| // projects, versions are truncated to major.minor before comparison so that e.g. | ||
| // "4.22.z" matches a configured "4.22.0" (both truncate to "4.22"). | ||
| func dependentTargetVersionMatch(actualVersion string, configuredVersions []string, parentBug *jira.Issue) bool { | ||
| projectKey := "" | ||
| if parentBug.Fields != nil { | ||
| projectKey = parentBug.Fields.Project.Key | ||
| } | ||
| if projectKey == "DFBUGS" { | ||
| return sets.NewString(configuredVersions...).Has(actualVersion) | ||
| } | ||
| truncatedActual := truncateVersionToMajorMinor(actualVersion) | ||
| for _, configured := range configuredVersions { | ||
| if truncateVersionToMajorMinor(configured) == truncatedActual { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
A lot of this code is shared with validateTargetVersion (such as the truncation and DFBUGS project check). We should have dependentTargetVersionMatch and validateTargetVersion share the validation code instead of duplicating it.
There was a problem hiding this comment.
Good call — refactored in the latest push (commit 97dd2f0):
- Extracted
isExactVersionMatchProject()— shared helper that encapsulates the DFBUGS project check with nil-safe Fields access, now used by bothvalidateTargetVersionanddependentTargetVersionMatch. validateTargetVersionnow reusestruncateVersionToMajorMinor()instead of manually inlining thestrings.Split+fmt.Sprintftruncation logic.- Behavioral differences preserved:
validateTargetVersionstill usesstrings.HasPrefix(single primary bug), whiledependentTargetVersionMatchuses equality on truncated versions (checking against a list). - All existing tests pass unchanged — pure refactor, no behavior change.
AI-generated. Review for accuracy.
393e58f to
97dd2f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/jira-lifecycle-plugin/server.go`:
- Around line 1725-1729: Update the target-version validation around
isExactVersionMatchProject(issue) so DFBUGS compares targetVersion[0].Name
directly to requiredTargetVersion with equality, rejecting suffix and openshift-
variants before the existing non-DFBUGS prefix checks. Preserve the current
prefix-based behavior for non-DFBUGS projects and add rejection tests for both
invalid variants.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e088cbc1-e092-40a8-93e9-0a832d191f79
📒 Files selected for processing (2)
cmd/jira-lifecycle-plugin/server.gocmd/jira-lifecycle-plugin/server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/jira-lifecycle-plugin/server_test.go
97dd2f0 to
9581b8b
Compare
| truncatedRequiredTargetVersion := truncateVersionToMajorMinor(requiredTargetVersion) | ||
| truncatedPrefixedRequiredTargetVersion := fmt.Sprintf("openshift-%s", truncatedRequiredTargetVersion) | ||
| if !strings.HasPrefix(targetVersion[0].Name, truncatedRequiredTargetVersion) && !strings.HasPrefix(targetVersion[0].Name, truncatedPrefixedRequiredTargetVersion) { | ||
| return fmt.Errorf("expected the %s to target either version %q or %q, but it targets %q instead", issueType, fmt.Sprintf("%s.*", truncatedRequiredTargetVersion), fmt.Sprintf("%s.*", truncatedPrefixedRequiredTargetVersion), targetVersion[0].Name) |
There was a problem hiding this comment.
This can be simplified to
if truncateVersionToMajorMinor(requiredTargetVersion) != truncateVersionMajorToMinor(targetVersion[0].name) {
return fmt.Errorf("expected the %s to target either version %s.* or openshift-%s.*, but it targets %q instead", issueType, requiredTargetVersion, requiredTargetVersion, targetVersion[0].Name)
There was a problem hiding this comment.
Done — simplified in the latest push. The non-DFBUGS path now uses:
if truncateVersionToMajorMinor(requiredTargetVersion) != truncateVersionToMajorMinor(targetVersion[0].Name) {
return fmt.Errorf("expected the %s to target either version %s.* or openshift-%s.*, but it targets %q instead",
issueType, requiredTargetVersion, requiredTargetVersion, targetVersion[0].Name)
}
return nilReplaced the multi-line HasPrefix + openshift- prefix block with a two-line truncate-both-sides comparison. All tests pass.
AI-generated. Review for accuracy.
There was a problem hiding this comment.
Put the "%s." and "openshift-%s." in quotes for clarity and so that the message matches the previous behavior.
There was a problem hiding this comment.
Done — quoted the version patterns in the latest push: "%s.*" and "openshift-%s.*". Updated the matching test expectations as well.
AI-generated. Review for accuracy.
|
/retest-required AI-generated. Review for accuracy. |
9581b8b to
bba6a19
Compare
|
/retest AI-generated. Review for accuracy. |
The dependent bug target version check used strict set membership, requiring an exact match against DependentBugTargetVersions. This caused false negatives once a release went GA and Jira target versions changed from X.Y.0 to X.Y.z (e.g. 4.22.0 → 4.22.z). Align with validateTargetVersion() by truncating both the configured and actual versions to major.minor before comparing. The openshift- prefix is also stripped for consistency. DFBUGS projects retain exact matching.
bba6a19 to
cd03cc1
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexNPavel, redhat-chai-bot 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 |
openshift-ci
Bot
added
the
approved
4 participants
Summary
Fix dependent bug target version validation to use prefix matching (major.minor) instead of strict set membership. This aligns the dependent bug check with how
validateTargetVersion()already handles primary bug versions.Problem
When validating backport cherry-pick PRs, the plugin checks that dependent bugs target a version listed in
DependentBugTargetVersions. This check uses strict string equality:Once a release goes GA, Jira target versions change from
X.Y.0toX.Y.z(e.g.4.22.0→4.22.z). The strict equality check rejects4.22.zwhen the config specifies4.22.0, even though they refer to the same release stream.Meanwhile, the primary bug's target version is validated using prefix matching in
validateTargetVersion(), which truncates tomajor.minorand usesstrings.HasPrefix()— so both4.22.0and4.22.zpass.This inconsistency causes false negatives for any backport PR once the parent version goes GA. For example: openshift/hypershift#9263 (comment)
Fix
truncateVersionToMajorMinor()helper that truncates4.22.0→4.22dependentTargetVersionMatch()that truncates both the actual and configured versions tomajor.minorbefore comparingvalidateTargetVersion()Testing
7 new test cases in
TestValidateBug:4.22.zmatches configured4.22.0✅4.22.0matches configured4.22.z✅4.23.0does NOT match4.22.0✅4.22.0=4.22.0still works ✅All existing tests continue to pass.
AI-generated. Review for accuracy.
@bryan-cox requested in Slack thread
Summary by CodeRabbit
openshift-prefixed variants.