prowgen: allow per-test slack reporter config in ci-operator config#5149
prowgen: allow per-test slack reporter config in ci-operator config#5149Prucek wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughAdds Slack reporter configuration types and defaults to the API, threads per-test Slack reporter settings through prowgen job generation (periodic/presubmit/postsubmit) with presubmit gating, updates validation to enforce reporter constraints, and adds integration test input/output YAMLs exercising the behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Couldn't analyze Linked repositories: Couldn't analyze Linked repositories: Couldn't analyze Linked repositories: Couldn't analyze Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
Bot
added
the
approved
|
Scheduling tests matching the |
Prucek
force-pushed
the
prowgen-slack-reporter
branch
from
0dc3372 to
978d30b
Compare
![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 the current code and only fix it if needed.
Inline comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 269-285: The code treats any matching testSlackConfig as
authoritative even when its Channel is empty, causing the fallback config to be
skipped; modify the conditional to require a non-empty testSlackConfig.Channel
(or explicitly check for Channel == "") before returning so that if Channel is
empty you continue to the legacy .config.prowgen fallback; adjust the block
around slackConfigAppliesToJobType(testSlackConfig, jobType) to only construct
and return a prowv1.ReporterConfig with SlackReporterConfig when
testSlackConfig.Channel is non-empty (while still applying the existing defaults
cioperatorapi.DefaultSlackReporterJobStatesToReport and
cioperatorapi.DefaultSlackReporterReportTemplate).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0d6c9bbc-65b8-42e2-97b9-8bb090d522bb
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (6)
pkg/api/types.gopkg/prowgen/prowgen.gotest/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
✅ Files skipped from review due to trivial changes (1)
- test/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yaml
| if testSlackConfig != nil && slackConfigAppliesToJobType(testSlackConfig, jobType) { | ||
| jobStatesToReport := testSlackConfig.JobStatesToReport | ||
| if len(jobStatesToReport) == 0 { | ||
| jobStatesToReport = cioperatorapi.DefaultSlackReporterJobStatesToReport | ||
| } | ||
| reportTemplate := testSlackConfig.ReportTemplate | ||
| if reportTemplate == "" { | ||
| reportTemplate = cioperatorapi.DefaultSlackReporterReportTemplate | ||
| } | ||
| return &prowv1.ReporterConfig{ | ||
| Slack: &prowv1.SlackReporterConfig{ | ||
| Channel: testSlackConfig.Channel, | ||
| JobStatesToReport: jobStatesToReport, | ||
| ReportTemplate: reportTemplate, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t let an empty per-test channel suppress the legacy fallback.
Line 269 treats any matching per-test reporter_config as authoritative, even when channel is empty. That skips .config.prowgen fallback and generates jobs with a broken Slack reporter during migration.
Suggested fix
-func slackReporterConfig(jobName, testName string, jobType prowv1.ProwJobType, testSlackConfig *cioperatorapi.SlackReporterConfig, info *ProwgenInfo) *prowv1.ReporterConfig {
- if testSlackConfig != nil && slackConfigAppliesToJobType(testSlackConfig, jobType) {
+func slackReporterConfig(jobName, testName string, jobType prowv1.ProwJobType, testSlackConfig *cioperatorapi.SlackReporterConfig, info *ProwgenInfo) *prowv1.ReporterConfig {
+ if testSlackConfig != nil && slackConfigAppliesToJobType(testSlackConfig, jobType) && testSlackConfig.Channel != "" {
jobStatesToReport := testSlackConfig.JobStatesToReport
if len(jobStatesToReport) == 0 {
jobStatesToReport = cioperatorapi.DefaultSlackReporterJobStatesToReport
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/prowgen/prowgen.go` around lines 269 - 285, The code treats any matching
testSlackConfig as authoritative even when its Channel is empty, causing the
fallback config to be skipped; modify the conditional to require a non-empty
testSlackConfig.Channel (or explicitly check for Channel == "") before returning
so that if Channel is empty you continue to the legacy .config.prowgen fallback;
adjust the block around slackConfigAppliesToJobType(testSlackConfig, jobType) to
only construct and return a prowv1.ReporterConfig with SlackReporterConfig when
testSlackConfig.Channel is non-empty (while still applying the existing defaults
cioperatorapi.DefaultSlackReporterJobStatesToReport and
cioperatorapi.DefaultSlackReporterReportTemplate).
978d30b to
f7fadaf
Compare
Add a `reporter_config` field to `TestStepConfiguration` that lets ci-operator config files specify Slack notifications directly on individual tests, with an optional `job_types` filter to control which generated job types (presubmit/postsubmit/periodic) receive the config. Per-test config takes precedence over `.config.prowgen` slack_reporter, which remains as a fallback during the migration period. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prucek
force-pushed
the
prowgen-slack-reporter
branch
from
f7fadaf to
70184f0
Compare
|
/test e2e |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/prowgen/prowgen.go (1)
227-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t use
nilto mean “suppress Slack completely” for periodic-derived presubmits.Line 229 clears the inline config, but Line 312 still routes through
slackReporterConfig(...), andnilthere falls through to.config.prowgenon Lines 290-297. So a periodic test withpresubmit: truecan still inherit the legacy fallback reporter even whenreport_presubmitis false or unset, which breaks the new migration gating behavior.Also applies to: 272-299, 302-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/prowgen/prowgen.go` around lines 227 - 243, The code clears slackConfig by setting it to nil to suppress Slack for periodic-derived presubmits, but nil is interpreted later by slackReporterConfig(...) and causes an undesired fallback to the legacy .config.prowgen; instead, explicitly pass a non-nil SlackReporterConfig that indicates "do not report presubmits." Change the branch that currently does "slackConfig = nil" to assign a new cioperatorapi.SlackReporterConfig value with ReportPresubmit set to false (and preserve any other needed fields if present) so generatePresubmitForTest(...) / slackReporterConfig(...) receives an explicit config and will not fall back to the legacy reporter; update the same pattern for the other occurrences around the handlePresubmit flow (the slackConfig variable, the generatePresubmitForTest call, and any paths that call slackReporterConfig(...)).
🧹 Nitpick comments (1)
test/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yaml (1)
43-61: ⚡ Quick winAdd a fixture for the legacy fallback migration path.
This input proves inline
reporter_configbehavior, but it does not cover a periodic+presubmit test that gets Slack settings only from.config.prowgen. Without that case, thereport_presubmitgate can regress while the inline path still passes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yaml` around lines 43 - 61, Add a fixture that exercises the legacy fallback (periodic+presubmit) path by adding a job entry that has cron: '@daily' and presubmit: true but does NOT include an inline reporter_config so the Slack settings must be read from .config.prowgen; e.g., add a new job block (name it verify-legacy or similar) with as: verify-legacy, commands: make verify, container.from: src, cron: '@daily', presubmit: true and omit reporter_config and report_presubmit keys so the code path that falls back to .config.prowgen is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/prowgen/prowgen.go`:
- Around line 227-243: The code clears slackConfig by setting it to nil to
suppress Slack for periodic-derived presubmits, but nil is interpreted later by
slackReporterConfig(...) and causes an undesired fallback to the legacy
.config.prowgen; instead, explicitly pass a non-nil SlackReporterConfig that
indicates "do not report presubmits." Change the branch that currently does
"slackConfig = nil" to assign a new cioperatorapi.SlackReporterConfig value with
ReportPresubmit set to false (and preserve any other needed fields if present)
so generatePresubmitForTest(...) / slackReporterConfig(...) receives an explicit
config and will not fall back to the legacy reporter; update the same pattern
for the other occurrences around the handlePresubmit flow (the slackConfig
variable, the generatePresubmitForTest call, and any paths that call
slackReporterConfig(...)).
---
Nitpick comments:
In
`@test/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yaml`:
- Around line 43-61: Add a fixture that exercises the legacy fallback
(periodic+presubmit) path by adding a job entry that has cron: '@daily' and
presubmit: true but does NOT include an inline reporter_config so the Slack
settings must be read from .config.prowgen; e.g., add a new job block (name it
verify-legacy or similar) with as: verify-legacy, commands: make verify,
container.from: src, cron: '@daily', presubmit: true and omit reporter_config
and report_presubmit keys so the code path that falls back to .config.prowgen is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f2793994-991e-4fbd-a4da-c188c5450948
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (7)
pkg/api/types.gopkg/prowgen/prowgen.gopkg/validation/test.gotest/integration/ci-operator-prowgen/input/config/slack-report-inline/duper/slack-report-inline-duper-master.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yamltest/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-presubmits.yaml
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-periodics.yaml
- test/integration/ci-operator-prowgen/output/jobs/slack-report-inline/duper/slack-report-inline-duper-master-postsubmits.yaml
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/override ci/prow/images |
|
/label ready-for-human-review |
openshift-ci
Bot
added
the
ready-for-human-review
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/e2e, ci/prow/images DetailsIn response to this:
Instructions 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. |
|
@Prucek: 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. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
3 similar comments
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
1 participant
Summary
reporter_configfield toTestStepConfigurationso ci-operator config files can specify Slack notifications directly on individual tests.config.prowgenslack_reporter, which remains as fallback during the migrationjob_states_to_report: [success, failure, error]and a standard report template matching the most common pattern in the release reposlackReporterConfig()returns*prowv1.ReporterConfiginstead of mutating the job basepresubmit: true, the presubmit job does not get the slack config by default — setreport_presubmit: trueon the reporter config to include itchannelis required,report_presubmitcan only be set whenpresubmit: true.config.prowgendeprecation🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests