WIP: INTEROP-8979: Wire Slack webhook into firewatch-report-issues step#78426
WIP: INTEROP-8979: Wire Slack webhook into firewatch-report-issues step#78426amp-rh wants to merge 11 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@amp-rh: This pull request references INTEROP-8979 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
from
e6707ba to
fb4f23e
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces Slack webhook notification functionality to the firewatch CI/CD system by adding a new test configuration, mounting webhook credentials as a secret, and updating the report-issues step to load and validate webhook URLs before posting notifications to external services. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as CI Test Job
participant FirewatchStep as Firewatch Report-Issues Step
participant Secret as Secret Mount
participant Slack as Slack Webhook API
participant Jira as Jira API
Test->>FirewatchStep: Execute as post-ref after test failure
FirewatchStep->>Secret: Read webhook URL from /tmp/secrets/slack
Secret-->>FirewatchStep: Return webhook URL
FirewatchStep->>FirewatchStep: Validate webhook URL non-empty
FirewatchStep->>Slack: POST issue notification with webhook URL
Slack-->>FirewatchStep: Webhook delivery confirmation
FirewatchStep->>Jira: Create/update issue in INTEROP project
Jira-->>FirewatchStep: Issue creation response
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 12✅ 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🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
from
fb4f23e to
5aec3ab
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: 2
🧹 Nitpick comments (2)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml (1)
27-30: Minor: hardcoded personal email in tracked config.
mpruitt@redhat.comis hardcoded as both the Slack notification target and the default Jira assignee. This is a common pattern in CI configs but couples the test to a single individual — if that person changes roles, the test will silently route to a stale identity. Consider using a team alias or distribution list (e.g., a team Jira account / Slack group handle) so ownership transfers don't require config edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml` around lines 27 - 30, Replace the hardcoded personal address used for Slack and Jira ownership: change the "slack_user": "mpruitt@redhat.com" entry in the notification rule and the FIREWATCH_DEFAULT_JIRA_ASSIGNEE: mpruitt@redhat.com value to a team alias or distribution account (e.g., a Slack channel handle or team Jira account) so ownership is not tied to a single individual; update the JSON rule and the FIREWATCH_DEFAULT_JIRA_ASSIGNEE variable to the chosen team identifier and ensure any consumers expect that format.ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh (1)
45-49: Optional: simplify the trim withread -r.The trailing-whitespace trim
${SLACK_WEBHOOK_URL%"${SLACK_WEBHOOK_URL##*[![:space:]]}"}is correct but hard to maintain. Usingread -rreads the first line directly, naturally dropping the trailing newline that secret files typically contain, and is a more idiomatic shell pattern.♻️ Proposed simplification
if [ -f /tmp/secrets/slack/slack_rule_notification_webhook_url ]; then - SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/slack_rule_notification_webhook_url) - SLACK_WEBHOOK_URL="${SLACK_WEBHOOK_URL%"${SLACK_WEBHOOK_URL##*[![:space:]]}"}" - export SLACK_WEBHOOK_URL + read -r SLACK_WEBHOOK_URL < /tmp/secrets/slack/slack_rule_notification_webhook_url + export SLACK_WEBHOOK_URL fiAlso note: if the secret file is empty (e.g., the credential exists but has no content), this block silently exports an empty
SLACK_WEBHOOK_URL. That's acceptable since the ref's default is also empty, but worth keeping in mind if a caller ever sets the env directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh` around lines 45 - 49, Replace the cat + parameter-expansion trailing-space trim inside the if block that reads the secret with a simple read -r into SLACK_WEBHOOK_URL: instead of using SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/slack_rule_notification_webhook_url) and the complex trim, use read -r to read the first line directly into SLACK_WEBHOOK_URL (preserving the conditional test that the file exists) and then export SLACK_WEBHOOK_URL; this simplifies maintenance and naturally removes the trailing newline from the secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml`:
- Line 19: The cron schedule "cron: 0 23 31 2 *" in the
RedHatQE-firewatch-main__slack-webhook-test.yaml is a never-run (Feb 31)
rehearsal pattern; confirm the intent is manual/rehearsal-only and, if so, add
an inline comment or a "# disabled-by-design" marker next to the cron entry to
prevent future maintainers from "fixing" it, and update README or a comment to
explicitly warn that the job's step fail-to-trigger-webhook provisions a full
AWS IPI cluster, creates a Jira ticket in INTEROP (assigned to
mpruitt@redhat.com) and posts to Slack so rehearsers are aware.
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml`:
- Around line 15-17: The ref firewatch-report-issues currently mounts the secret
firewatch-slack-notify-webhook-url (key slack_rule_notification_webhook_url)
from namespace test-credentials at /tmp/secrets/slack unconditionally, which
will break scheduling if that secret is missing; either ensure the secret is
added to the secret-bootstrap provisioning for test-credentials (so the key
slack_rule_notification_webhook_url is present) OR remove the unconditional
mount from the ci-operator ref and instead add the secret only to the new
slack-webhook-test variant (or make the mount optional) so existing consumers
aren’t forced to have the secret; confirm by verifying secret-bootstrap contains
firewatch-slack-notify-webhook-url and by running an unrelated job that uses
firewatch-report-issues to ensure no regressions.
---
Nitpick comments:
In
`@ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml`:
- Around line 27-30: Replace the hardcoded personal address used for Slack and
Jira ownership: change the "slack_user": "mpruitt@redhat.com" entry in the
notification rule and the FIREWATCH_DEFAULT_JIRA_ASSIGNEE: mpruitt@redhat.com
value to a team alias or distribution account (e.g., a Slack channel handle or
team Jira account) so ownership is not tied to a single individual; update the
JSON rule and the FIREWATCH_DEFAULT_JIRA_ASSIGNEE variable to the chosen team
identifier and ensure any consumers expect that format.
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh`:
- Around line 45-49: Replace the cat + parameter-expansion trailing-space trim
inside the if block that reads the secret with a simple read -r into
SLACK_WEBHOOK_URL: instead of using SLACK_WEBHOOK_URL=$(cat
/tmp/secrets/slack/slack_rule_notification_webhook_url) and the complex trim,
use read -r to read the first line directly into SLACK_WEBHOOK_URL (preserving
the conditional test that the file exists) and then export SLACK_WEBHOOK_URL;
this simplifies maintenance and naturally removes the trailing newline from the
secret.
🪄 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: 73fc4b1a-a3c0-4d72-bbbb-ef090971514b
⛔ Files ignored due to path filters (1)
ci-operator/jobs/RedHatQE/firewatch/RedHatQE-firewatch-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (5)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yamlci-operator/config/openshift/ci-tools/openshift-ci-tools-main.yamlci-operator/step-registry/cluster-profiles/cluster-profiles-config.yamlci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.shci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml
💤 Files with no reviewable changes (1)
- ci-operator/config/openshift/ci-tools/openshift-ci-tools-main.yaml
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
2 times, most recently
from
1eb0516 to
6e5b522
Compare
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
amp-rh
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
from
6e5b522 to
72f4c18
Compare
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: job(s): periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test either don't exist or were not found to be affected, and cannot be rehearsed |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
from
72f4c18 to
ee817c0
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml (1)
15-17:⚠️ Potential issue | 🟠 MajorUnconditional Slack credential mount can still break existing consumers
At Line 15-17, this mount remains mandatory for every
firewatch-report-issuesconsumer. Iffirewatch-slack-notify-webhook-urlis missing intest-credentials, jobs fail before the opt-in runtime logic infirewatch-report-issues-commands.sh(Line 45-53) executes. Please either provision it universally before merge or scope this credential to the webhook test variant only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml` around lines 15 - 17, The manifest unconditionally mounts the secret firewatch-slack-notify-webhook-url at /tmp/secrets/slack causing jobs to fail if that secret is absent; make the secret mount optional by scoping it to the webhook test variant or gating it behind the webhook-specific job variant so only the webhook variant includes the secret mount, and update the consumer spec in firewatch-report-issues-ref.yaml accordingly; reference the secret name firewatch-slack-notify-webhook-url, the mount_path /tmp/secrets/slack, and the runtime opt-in logic in firewatch-report-issues-commands.sh (lines ~45-53) when applying the conditional change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml`:
- Around line 15-17: The manifest unconditionally mounts the secret
firewatch-slack-notify-webhook-url at /tmp/secrets/slack causing jobs to fail if
that secret is absent; make the secret mount optional by scoping it to the
webhook test variant or gating it behind the webhook-specific job variant so
only the webhook variant includes the secret mount, and update the consumer spec
in firewatch-report-issues-ref.yaml accordingly; reference the secret name
firewatch-slack-notify-webhook-url, the mount_path /tmp/secrets/slack, and the
runtime opt-in logic in firewatch-report-issues-commands.sh (lines ~45-53) when
applying the conditional change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3f4b4cf1-a956-4ff4-a1be-62cd9ed44d69
⛔ Files ignored due to path filters (1)
ci-operator/jobs/RedHatQE/firewatch/RedHatQE-firewatch-main-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (5)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yamlci-operator/config/openshift/ci-tools/openshift-ci-tools-main.yamlci-operator/step-registry/cluster-profiles/cluster-profiles-config.yamlci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.shci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml
💤 Files with no reviewable changes (1)
- ci-operator/config/openshift/ci-tools/openshift-ci-tools-main.yaml
✅ Files skipped from review due to trivial changes (2)
- ci-operator/step-registry/cluster-profiles/cluster-profiles-config.yaml
- ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v3
branch
from
ee817c0 to
31eee68
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amp-rh The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
/pj-rehearse pull-ci-RedHatQE-firewatch-main-slack-webhook-test-images |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh, If the problem persists, please contact Test Platform. |
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
The commands field in an inline step is executed as a literal shell command, not resolved from the step registry. Replace the filename reference with the actual script content.
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
…ream The step name firewatch-report-issues collided with the registered step ref, causing ci-operator to use the ref's from_image (published firewatch:main imagestream) instead of the pipeline image built from the fork branch. Renaming to firewatch-report-issues-fork breaks the ref association so from: main correctly resolves to the pipeline image.
|
/pj-rehearse pull-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: job(s): pull-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test either don't exist or were not found to be affected, and cannot be rehearsed |
|
/pj-rehearse pull-ci-RedHatQE-firewatch-main-slack-webhook-test-images |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Writes a marker file during image build and checks for it at step runtime. Also verifies Report._notify_failure_webhooks exists in the installed code. This will definitively show whether the step uses the pipeline image or the published imagestream.
|
/pj-rehearse pull-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse pull-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: job(s): pull-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test either don't exist or were not found to be affected, and cannot be rehearsed |
|
/pj-rehearse periodic-ci-RedHatQE-firewatch-main-slack-webhook-test-slack-webhook-test |
|
@amp-rh: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@amp-rh: The following tests 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 Slack notification approach from this PR and its dependency (RedHatQE/firewatch#274) has been replaced with a simpler design in RedHatQE/firewatch#277. Instead of posting Slack messages directly from firewatch (which required wiring This eliminates the need for:
This PR can be closed once RedHatQE/firewatch#277 is merged. |
2 participants
Summary
firewatch-slack-notify-webhook-urlsecret at/tmp/secrets/slackin thefirewatch-report-issuesstep/tmp/secrets/slack/slack_rule_notification_webhook_urland export as$SLACK_WEBHOOK_URLbefore running firewatch reportSLACK_WEBHOOK_URLenv var declaration to the step refRedHatQE-firewatch-main__slack-webhook-test.yaml) to verify end-to-end webhook deliveryDependencies
Requires firewatch PR: RedHatQE/firewatch#274
Prerequisites
firewatch-slack-notify-webhook-urlsecret intest-credentialsnamespace with keyslack_rule_notification_webhook_urlTest plan
slack-webhook-testjob from this PRSupersedes: #78342
Relates: https://issues.redhat.com/browse/INTEROP-8979
Summary by CodeRabbit