fix(config-brancher): preserve presubmit tests when using --skip-periodics#4837
fix(config-brancher): preserve presubmit tests when using --skip-periodics#4837petr-muller wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughUpdated removePeriodics in cmd/config-brancher to keep tests that are both periodic and presubmit by clearing periodic scheduling fields ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a bug in the config-brancher tool where the --skip-periodics option was incorrectly removing test definitions that served dual purposes (both periodic and presubmit). The fix ensures these tests are preserved as presubmit jobs on the destination branch by removing only their periodic-related fields.
Key Changes:
- Modified
removePeriodics()function to preserve tests that have both periodic and presubmit characteristics - Added comprehensive test coverage for the new branching behavior with dual-purpose tests
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/config-brancher/main.go | Updated removePeriodics() to preserve presubmit tests by clearing only periodic fields instead of removing the entire test definition |
| cmd/config-brancher/main_test.go | Added test case validating that dual-purpose tests are correctly preserved as presubmit-only tests when branching with --skip-periodics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test.Interval = nil | ||
| test.MinimumInterval = nil | ||
| test.ReleaseController = false | ||
| test.Presubmit = false |
There was a problem hiding this comment.
Setting Presubmit to false defeats the purpose of preserving presubmit tests. According to the PR description, these tests should continue to exist as presubmit jobs on the destination branch. This line should be removed to keep Presubmit: true.
| test.Presubmit = false |
There was a problem hiding this comment.
This stanza is only used when the job would otherwise be generated only as a periodic. Without any special stanzas, generating a presubmit job is the default behavior that does not need to be expliitly enabled.
|
Scheduling required tests: Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller, Prucek 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 |
|
/hold I'd like to see what |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
|
/remove-lifecycle rotten |
…odics The --skip-periodics option was incorrectly removing test definitions that were both periodic and presubmit (tests with both a cron/interval stanza and presubmit: true). This fix modifies the removePeriodics function to preserve these tests by removing only the periodic-related fields (Cron, Interval, MinimumInterval, ReleaseController, Presubmit) while keeping the test definition itself, so it continues to run as a presubmit job on the destination branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
33202fd to
a6426fb
Compare
|
New changes are detected. LGTM label has been removed. |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
@petr-muller: The following test failed, say
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. |
The --skip-periodics option was incorrectly removing test definitions that were both periodic and presubmit (tests with both a cron/interval stanza and presubmit: true). This fix modifies the removePeriodics function to preserve these tests by removing only the periodic-related fields (Cron, Interval, MinimumInterval, ReleaseController, Presubmit)
while keeping the test definition itself, so it continues to exist as a presubmit job on the destination branch, and we do not leave such presubmit behind when we branch.
xref: openshift/release#70913 (comment)
🤖 Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com