Skip to content

Fix Slack channel to use saas configured channel#3148

Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
YiqinZhang:slack-channel
Mar 12, 2026
Merged

Fix Slack channel to use saas configured channel#3148
openshift-merge-bot[bot] merged 2 commits intoopenshift:mainfrom
YiqinZhang:slack-channel

Conversation

@YiqinZhang
Copy link
Contributor

@YiqinZhang YiqinZhang commented Mar 11, 2026

Fixes Slack notifications not being sent for ad-hoc test suite failures when LLM log analysis is disabled.

Decouple notification queueing from EnableAnalysis flag — queueNotification is always called on failure
sendFailureNotification uses SLACK_CHANNEL (SaaS parameter) directly for channel resolution
Per-suite and global notifications are mutually exclusive to prevent duplicates

Notification paths

Scenario Per-suite notification Global notification
No suites, test failed None (queue empty) Sent
Suites configured, test failed, analysis on Sent (with LLM content) Skipped
Suites configured, test failed, analysis off Sent (without LLM content) Skipped
Provision failed, suites configured None (tests never ran) Sent (fallback)
Provision failed, no suites None Sent

SDCICD-1758

@openshift-ci-robot
Copy link

There are test jobs defined for this repository which are not configured to run automatically. Comment /test ? to see a list of all defined jobs. Review these jobs and use /test <job> to manually trigger jobs most likely to be impacted by the proposed changes.Comment /pipeline required to trigger all required & necessary jobs.

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 8dfab127-7791-4eec-a80a-ea237ac4c466

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can enforce grammar and style rules using `languagetool`.

Configure the reviews.tools.languagetool setting to enable/disable rules and categories. Refer to the LanguageTool Community to learn more.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 11, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: YiqinZhang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2026
@YiqinZhang YiqinZhang changed the title read slack para first Fix Slack channel to use configured parameter Mar 11, 2026
@YiqinZhang YiqinZhang changed the title Fix Slack channel to use configured parameter Fix Slack channel to use test yaml configured channel Mar 11, 2026
@YiqinZhang YiqinZhang changed the title Fix Slack channel to use test yaml configured channel Fix Slack channel to use saas configured channel Mar 11, 2026
@YiqinZhang YiqinZhang force-pushed the slack-channel branch 2 times, most recently from 69ebda8 to cf88553 Compare March 11, 2026 22:15
@YiqinZhang
Copy link
Contributor Author

/pipeline required

@openshift-ci-robot
Copy link

Scheduling required tests:
/test hypershift-pr-check

pkg/e2e/e2e.go Outdated
Copy link
Contributor

@ritmun ritmun Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping parity with original logic in this prior to separating slack from log analysis:
Do we need one more conditional here so we skip this for suite level errors?
viper.GetString(config.Tests.TestSuites) == "" && viper.GetString(config.Tests.AdHocTestImages) == ""

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case is handled in sendDeferredNotifications().

Scenario Per-suite notification Global notification
No suites configured, test failed None (queue empty) → false Sent
No suites configured, test passed None → false Skipped (ExitCode == Success)
Suites configured, tests ran and failed Sent → true Skipped (deferredSent=true)
Suites configured, tests ran and passed Sent → true Skipped
Provision failed, suites configured None (tests never ran) → false Sent (fallback)

@YiqinZhang
Copy link
Contributor Author

/test hypershift-pr-check

@ritmun
Copy link
Contributor

ritmun commented Mar 12, 2026

I think this update is not fixing the issue of suite level slacks not going out.

The pending collection of notifications is staying empty, because it's still dependent on llm enabled flag, and it's set to false. This is why test suite notifications(deferred) aren't going out. Slack should be independent of log analysis per https://issues.redhat.com/browse/SDCICD-1767.

Continuing discussion in slack

Comment on lines +134 to +138
if viper.GetBool(config.LogAnalysis.EnableAnalysis) {
runLogAnalysisForAdHocTestImage(ctx, logger, testSuite, combinedErr, exeConfig.OutputDir)
} else {
queueNotification(testSuite, "")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if viper.GetBool(config.LogAnalysis.EnableAnalysis) {
runLogAnalysisForAdHocTestImage(ctx, logger, testSuite, combinedErr, exeConfig.OutputDir)
} else {
queueNotification(testSuite, "")
}
// is this possible?
additionalContent := ""
if viper.GetBool(config.LogAnalysis.EnableAnalysis) {
// get this returned from function
additionalContent = runLogAnalysisForAdHocTestImage(ctx, logger, testSuite, combinedErr, exeConfig.OutputDir)
}
queueNotification(testSuite, additionalContent)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach will work, with one suggestion for more concise flow.

@YiqinZhang YiqinZhang force-pushed the slack-channel branch 2 times, most recently from 481c386 to f518093 Compare March 12, 2026 18:22
@ritmun
Copy link
Contributor

ritmun commented Mar 12, 2026

/override ci/prow/hypershift-pr-check

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 12, 2026

@ritmun: Overrode contexts on behalf of ritmun: ci/prow/hypershift-pr-check

Details

In response to this:

/override ci/prow/hypershift-pr-check

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 12, 2026

@YiqinZhang: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@YiqinZhang YiqinZhang requested a review from ritmun March 12, 2026 19:35
@ritmun
Copy link
Contributor

ritmun commented Mar 12, 2026

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2026
@openshift-merge-bot openshift-merge-bot bot merged commit 3fbe5fd into openshift:main Mar 12, 2026
6 checks passed
@ritmun
Copy link
Contributor

ritmun commented Mar 12, 2026

related to SDCICD-1767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants