[WIP] SDCICD-1785 various updates to slack message after enabling it on more operators#3167
[WIP] SDCICD-1785 various updates to slack message after enabling it on more operators#3167ritmun wants to merge 1 commit intoopenshift:mainfrom
Conversation
…e operators
- limit each workflow field to 2000 chars per existing limit due to 404 failures on large log dumps
- Slack Workflow Payload Updates:
- Added TektonURL field to WorkflowPayload for pipeline links
- Extracted repo and commit information separately from image string to format better in slack
- Removed redundant buildSummaryField and buildTestSuiteSection methods
- Changed cluster expiration timestamp format from default string to "Jan 02, 03:04 PM MST"
|
There are test jobs defined for this repository which are not configured to run automatically. Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
openshift-ci
bot
requested review from
christophermancini and
varunraokadaparthi
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ritmun 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
|
@ritmun: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/common/slack/reporter.go (1)
92-133:⚠️ Potential issue | 🔴 CriticalRemove or update test references to deleted methods.
Methods
buildSummaryFieldandbuildTestSuiteSectionhave been removed from the reporter but are still called in:
pkg/common/slack/reporter_workflow_test.go:203(buildSummaryField)pkg/common/slack/reporter_testoutput_test.go:370,388,402(buildTestSuiteSection)The tests will fail to compile. Either restore these methods or update the affected tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/common/slack/reporter.go` around lines 92 - 133, Tests still reference removed methods buildSummaryField and buildTestSuiteSection; update those tests to either call the current public builders or assert on the produced WorkflowPayload instead. Specifically, replace calls to buildSummaryField with building a payload via buildWorkflowPayload (or directly call buildAnalysisField) and assert on payload.Analysis, and replace calls to buildTestSuiteSection with calling buildArtifactLinksSection or reading payload.ExtendedLogs (via buildWorkflowPayload/readTestOutput) depending on intent; ensure assertions examine WorkflowPayload fields (Analysis, ExtendedLogs, ClusterDetails, Image, Commit, Env, TektonURL) rather than the deleted methods so the tests compile.
🧹 Nitpick comments (1)
pkg/e2e/e2e.go (1)
38-39: Hardcoded Tekton URL reduces portability.This URL is environment-specific. Consider externalizing to configuration (viper/config constant) to support different clusters.
♻️ Suggested approach
-const tektonurl = "https://console-openshift-console.apps.rosa.appsrep09ue1.03r5.p3.openshiftapps.com/pipelines/ns/%s-pipelines" +// TektonURL should be sourced from config, e.g.: +// viper.GetString(config.Tests.TektonURLTemplate)Define in
pkg/common/config/config.goand retrieve via viper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/e2e/e2e.go` around lines 38 - 39, The hardcoded tektonurl constant in pkg/e2e/e2e.go should be moved to configurable config (e.g., add a TektonURL key in pkg/common/config/config.go and load it via viper); change usages of the tektonurl constant to read from your config accessor (e.g., Config.GetString("tekton.url") or a typed Config.TektonURL field) and provide a sensible default or error if missing so tests remain portable across clusters.
🤖 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/common/slack/reporter.go`:
- Around line 119-130: Tests break because code now reads
config.Settings["repo"] and config.Settings["commit"] (populating payload.Image
and payload.Commit) while tests still set Settings["image"]; update the test
fixtures to provide Settings["repo"] and Settings["commit"] (and remove or
adjust Settings["image"] where appropriate) in the affected tests (e.g.,
TestSlackReporter_buildWorkflowPayload in reporter_workflow_test.go,
TestSlackReporter_Report_WorkflowFormat and related cases in
reporter_testoutput_test.go and slack_integration_test.go) so that payload.Image
and payload.Commit are populated for assertions against the payload built by the
Slack reporter.
In `@pkg/e2e/e2e.go`:
- Around line 383-386: extractDetailsFromImage can return an empty repo which
makes tektonns empty and produces a malformed URL; after setting
cfg.Settings["repo"] from extractDetailsFromImage and computing tektonns from
cfg.Settings["repo"], validate tektonns (and the underlying
cfg.Settings["repo"]) and substitute a safe fallback (e.g., "default" or
"unknown-repo") or return an error so tektonns is never empty before you build
cfg.Settings["tekton_url"]; update the block around extractDetailsFromImage,
tektonns, and cfg.Settings["tekton_url"] to sanitize/guard the value and
document the fallback behavior.
---
Outside diff comments:
In `@pkg/common/slack/reporter.go`:
- Around line 92-133: Tests still reference removed methods buildSummaryField
and buildTestSuiteSection; update those tests to either call the current public
builders or assert on the produced WorkflowPayload instead. Specifically,
replace calls to buildSummaryField with building a payload via
buildWorkflowPayload (or directly call buildAnalysisField) and assert on
payload.Analysis, and replace calls to buildTestSuiteSection with calling
buildArtifactLinksSection or reading payload.ExtendedLogs (via
buildWorkflowPayload/readTestOutput) depending on intent; ensure assertions
examine WorkflowPayload fields (Analysis, ExtendedLogs, ClusterDetails, Image,
Commit, Env, TektonURL) rather than the deleted methods so the tests compile.
---
Nitpick comments:
In `@pkg/e2e/e2e.go`:
- Around line 38-39: The hardcoded tektonurl constant in pkg/e2e/e2e.go should
be moved to configurable config (e.g., add a TektonURL key in
pkg/common/config/config.go and load it via viper); change usages of the
tektonurl constant to read from your config accessor (e.g.,
Config.GetString("tekton.url") or a typed Config.TektonURL field) and provide a
sensible default or error if missing so tests remain portable across clusters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3fb3144b-1451-49c0-acb8-8e99066ff2f0
📒 Files selected for processing (2)
pkg/common/slack/reporter.gopkg/e2e/e2e.go
| if image, ok := config.Settings["repo"].(string); ok { | ||
| payload.Image = image | ||
| parts := strings.Split(image, ":") | ||
| if len(parts) == 2 { | ||
| payload.Commit = parts[1] | ||
| } | ||
| } | ||
|
|
||
| if env, ok := config.Settings["env"].(string); ok && env != "" { | ||
| if commit, ok := config.Settings["commit"].(string); ok { | ||
| payload.Commit = commit | ||
| } | ||
| if env, ok := config.Settings["env"].(string); ok { | ||
| payload.Env = env | ||
| } | ||
| if tektonURL, ok := config.Settings["tekton_url"].(string); ok { | ||
| payload.TektonURL = tektonURL | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all test files setting "image" key in ReporterConfig Settings
rg -n '"image":\s*"' --glob '*_test.go' -C2Repository: openshift/osde2e
Length of output: 2748
🏁 Script executed:
# First, read the actual reporter.go file around lines 119-130 to verify the code snippet
head -n 140 pkg/common/slack/reporter.go | tail -n 40 | cat -nRepository: openshift/osde2e
Length of output: 1837
🏁 Script executed:
# Search for all code (not just tests) that sets "image" or "repo" in Settings
rg -n 'Settings\[.*"(image|repo)"' -C 1Repository: openshift/osde2e
Length of output: 757
🏁 Script executed:
# Check what the test files actually assert/validate
rg -n 'payload\.Image\|Image.*quay' pkg/common/slack/reporter_workflow_test.go pkg/common/slack/reporter_testoutput_test.go pkg/e2e/slack_integration_test.go -A 2 -B 2Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Check the test functions that use the "image" key to see what they verify
rg -n 'func.*Test' pkg/common/slack/reporter_workflow_test.go | head -20Repository: openshift/osde2e
Length of output: 627
🏁 Script executed:
# Read the first test function around line 41 where "image" key is used
sed -n '30,60p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 795
🏁 Script executed:
# Search for Image field assertions or checks in tests
rg -n 'payload\.Image|\.Image\s*==' pkg/common/slack/reporter_workflow_test.go pkg/common/slack/reporter_testoutput_test.go pkg/e2e/slack_integration_test.goRepository: openshift/osde2e
Length of output: 268
🏁 Script executed:
# Check the buildWorkflowPayload test more thoroughly - lines around the assertion section
sed -n '30,90p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 1901
🏁 Script executed:
# Check all instances where tests set "image" and what they expect in payload
rg -n 'Settings.*image|payload\.Image' pkg/common/slack/reporter_testoutput_test.go -B 5 -A 5Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Verify the Test assertion at line 92-93 in context - it will definitely fail now
sed -n '85,100p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 540
🏁 Script executed:
# Check all test files to see which ones have assertions on payload.Image or payload.Commit
rg -n 'payload\.(Image|Commit)' pkg/common/slack/reporter_testoutput_test.go pkg/e2e/slack_integration_test.go -B 2 -A 2Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Get the full test context to see what settings are provided vs what's asserted
sed -n '105,180p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 2266
🏁 Script executed:
# Verify the complete assertion section in the second test
sed -n '150,175p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 891
🏁 Script executed:
# Check reporter_testoutput_test.go for Image/Commit assertions
sed -n '360,410p' pkg/common/slack/reporter_testoutput_test.goRepository: openshift/osde2e
Length of output: 1253
🏁 Script executed:
# Check what extractDetailsFromImage does in e2e.go to understand the extraction logic
rg -n 'func extractDetailsFromImage' pkg/e2e/e2e.go -A 10Repository: openshift/osde2e
Length of output: 358
🏁 Script executed:
# Verify if buildTestSuiteSection reads from "image" or if it also needs the new keys
rg -n 'buildTestSuiteSection' pkg/common/slack/reporter.go -A 15Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Search for buildTestSuiteSection implementation in reporter.go
rg -n 'buildTestSuiteSection' pkg/common/slack/reporter.go -B 2 -A 20Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Check if tests are currently passing or failing - look for CI/test run info
git log --oneline -20 -- pkg/common/slack/reporter_workflow_test.go 2>/dev/null || echo "Git history not available"Repository: openshift/osde2e
Length of output: 149
🏁 Script executed:
# Verify if there's a different code path being called in tests vs production
rg -n 'buildWorkflowPayload|buildTestSuiteSection' pkg/common/slack/reporter.go -B 2Repository: openshift/osde2e
Length of output: 250
🏁 Script executed:
# Find where buildTestSuiteSection is implemented
fd -t f '*.go' -x rg -l 'buildTestSuiteSection' {}Repository: openshift/osde2e
Length of output: 343
🏁 Script executed:
# Get the full implementation of buildTestSuiteSection
rg -n 'func.*buildTestSuiteSection' -A 30Repository: openshift/osde2e
Length of output: 2393
🏁 Script executed:
# Find buildTestSuiteSection implementation in source code (not tests)
rg -n 'func.*buildTestSuiteSection' pkg/common/slack/reporter.go -A 30Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Also look for where it's called from buildWorkflowPayload
sed -n '92,130p' pkg/common/slack/reporter.goRepository: openshift/osde2e
Length of output: 1568
🏁 Script executed:
# Search more broadly for buildTestSuiteSection
rg -n 'buildTestSuiteSection' pkg/common/slack/Repository: openshift/osde2e
Length of output: 469
🏁 Script executed:
# Let's see the full reporter.go file structure
wc -l pkg/common/slack/reporter.goRepository: openshift/osde2e
Length of output: 92
🏁 Script executed:
# Now search for the function implementation
rg -n 'func (s \*SlackReporter) buildTestSuiteSection' pkg/common/slack/ -A 20Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Search for buildTestSuiteSection definition anywhere in the codebase
rg -n 'func.*buildTestSuiteSection' -A 20 --type goRepository: openshift/osde2e
Length of output: 1706
🏁 Script executed:
# Check if buildTestSuiteSection is being called in buildWorkflowPayload
rg -n 'buildTestSuiteSection\|buildSummaryField' pkg/common/slack/reporter.go -B 2 -A 5Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Look at the actual Summary field population in buildWorkflowPayload
rg -n 'payload\.Summary' pkg/common/slack/reporter.go -B 2 -A 2Repository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Let's get all lines from reporter.go to understand the flow better
cat -n pkg/common/slack/reporter.goRepository: openshift/osde2e
Length of output: 11959
🏁 Script executed:
# Verify what extractDetailsFromImage actually returns
sed -n '491,510p' pkg/e2e/e2e.goRepository: openshift/osde2e
Length of output: 579
🏁 Script executed:
# Check if there's any backward compatibility or fallback to Settings["image"] in reporter.go
rg -n 'Settings\["image"\]' pkg/common/slack/reporter.goRepository: openshift/osde2e
Length of output: 42
🏁 Script executed:
# Verify the test expectations - do they really check payload.Image against full image or just check it's not empty?
sed -n '88,100p' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 401
🏁 Script executed:
# Check how many tests will actually fail
rg -n 'payload\.Image\|payload\.Commit' pkg/common/slack/reporter_workflow_test.goRepository: openshift/osde2e
Length of output: 42
Tests will fail: Settings keys changed without updating test configurations.
Code at lines 119-130 reads from Settings["repo"] and Settings["commit"], but test files (reporter_workflow_test.go, reporter_testoutput_test.go, slack_integration_test.go) set only Settings["image"]. This causes test assertions for payload.Image and payload.Commit to fail with empty values.
Update test configurations to provide the required keys:
- Set
Settings["repo"](repository name) - Set
Settings["commit"](commit/tag reference) - Optionally remove
Settings["image"]if no longer needed
Affected tests: TestSlackReporter_buildWorkflowPayload (line 92-93), TestSlackReporter_Report_WorkflowFormat (line 164), and similar test cases across three test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/common/slack/reporter.go` around lines 119 - 130, Tests break because
code now reads config.Settings["repo"] and config.Settings["commit"] (populating
payload.Image and payload.Commit) while tests still set Settings["image"];
update the test fixtures to provide Settings["repo"] and Settings["commit"] (and
remove or adjust Settings["image"] where appropriate) in the affected tests
(e.g., TestSlackReporter_buildWorkflowPayload in reporter_workflow_test.go,
TestSlackReporter_Report_WorkflowFormat and related cases in
reporter_testoutput_test.go and slack_integration_test.go) so that payload.Image
and payload.Commit are populated for assertions against the payload built by the
Slack reporter.
| cfg.Settings["repo"], cfg.Settings["commit"] = extractDetailsFromImage(p.TestSuite.Image) | ||
| cfg.Settings["env"] = viper.GetString(ocmprovider.Env) | ||
| tektonns := strings.ReplaceAll(strings.ToLower(cfg.Settings["repo"].(string)), " ", "-") | ||
| cfg.Settings["tekton_url"] = fmt.Sprintf(tektonurl, tektonns) |
There was a problem hiding this comment.
Empty repo yields malformed Tekton URL.
If extractDetailsFromImage returns an empty repo (e.g., malformed image string), tektonns becomes empty, producing an invalid URL like .../ns/-pipelines.
🛡️ Guard against empty repo
cfg.Settings["repo"], cfg.Settings["commit"] = extractDetailsFromImage(p.TestSuite.Image)
-tektonns := strings.ReplaceAll(strings.ToLower(cfg.Settings["repo"].(string)), " ", "-")
-cfg.Settings["tekton_url"] = fmt.Sprintf(tektonurl, tektonns)
+if repo, ok := cfg.Settings["repo"].(string); ok && repo != "" {
+ tektonns := strings.ReplaceAll(strings.ToLower(repo), " ", "-")
+ cfg.Settings["tekton_url"] = fmt.Sprintf(tektonurl, tektonns)
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/e2e/e2e.go` around lines 383 - 386, extractDetailsFromImage can return an
empty repo which makes tektonns empty and produces a malformed URL; after
setting cfg.Settings["repo"] from extractDetailsFromImage and computing tektonns
from cfg.Settings["repo"], validate tektonns (and the underlying
cfg.Settings["repo"]) and substitute a safe fallback (e.g., "default" or
"unknown-repo") or return an error so tektonns is never empty before you build
cfg.Settings["tekton_url"]; update the block around extractDetailsFromImage,
tektonns, and cfg.Settings["tekton_url"] to sanitize/guard the value and
document the fallback behavior.
2 participants
example:
https://redhat-internal.slack.com/archives/C095XQANCBD/p1774042856278049