INTEROP-8979: Wire Slack webhook into firewatch-report-issues step#78342
INTEROP-8979: Wire Slack webhook into firewatch-report-issues step#78342amp-rh wants to merge 1 commit 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. |
|
/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. |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
WalkthroughAdds a new Firewatch CI config variant to test Slack webhook notifications, updates the firewatch report-issues step to optionally load a mounted Slack webhook secret and export Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI Job
participant Step as firewatch-report step
participant CLI as Firewatch CLI
participant Jira as Jira API
participant Slack as Slack Webhook
CI->>Step: start report-issues step (env + mounted secrets)
Step->>Step: if `/tmp/secrets/slack/url` exists -> read & export SLACK_WEBHOOK_URL
Step->>CLI: run `firewatch report` (env/FIREWATCH_CONFIG)
CLI->>Jira: create/annotate issue per config
CLI->>Slack: POST to SLACK_WEBHOOK_URL (if set)
Slack-->>CLI: response
Jira-->>CLI: response
CLI-->>Step: exit/status
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)
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
added
the
approved
|
@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 |
![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
🧹 Nitpick comments (2)
ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh (1)
45-47: Minor: splitexportfrom command substitution to avoid masking errors.Per ShellCheck SC2155,
export VAR=$(cmd)masks the exit status ofcmd, so a failedcathere would not triggererrexit. In practice the preceding[ -f ]check makes this very unlikely to matter, but splitting the two statements is the idiomatic fix and costs nothing.♻️ Proposed tweak
if [ -f /tmp/secrets/slack/url ]; then - export SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url) + export SLACK_WEBHOOK_URL fi🤖 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 - 47, Split the combined export and command substitution so the exit status of cat isn't masked: assign SLACK_WEBHOOK_URL using command substitution first (e.g., SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url)) and then run export SLACK_WEBHOOK_URL separately; keep the existing file-existence guard around the assignment (the symbols to change are SLACK_WEBHOOK_URL and the cat /tmp/secrets/slack/url command).ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml (1)
28-31: Heavyipi-installchain for a Slack-webhook-only test.Provisioning a full IPI AWS cluster in
prejust to run anechoand then thepostfirewatch-report step is expensive for what is effectively a webhook-delivery smoke test. If the goal is only to verifyfirewatch-report-issuesposts to Slack, consider dropping the cluster provisioning and using a lightweight workflow so the rehearsal is cheap and fast. If cluster context is required by firewatch report, please ignore.🤖 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 28 - 31, The pre-step currently includes a heavy cluster provisioning chain ("chain: ipi-install") alongside "ref: ipi-conf" and "ref: ipi-conf-aws", which makes this Slack-webhook smoke test expensive; remove or replace "chain: ipi-install" with a lightweight/no-op pre-chain (or omit it entirely) so the job only runs the minimal refs required, keeping "ref: ipi-conf"/"ref: ipi-conf-aws" if they are needed for configuration or else drop them and ensure the post step still invokes "firewatch-report-issues" as intended; if "firewatch-report-issues" truly requires cluster context, leave provisioning but otherwise remove "ipi-install" to make the rehearsal cheap and fast.
🤖 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`:
- Around line 18-25: Replace the hardcoded personal address used in
FIREWATCH_CONFIG (slack_user) and FIREWATCH_DEFAULT_JIRA_ASSIGNEE with a team
alias or channel/group distribution list for INTEROP (e.g., an `@channel` or dl)
so the config doesn't rely on a single person, and update the noop test to
actually exercise the webhook by either adding a deliberate failing step that
will trigger the "failure_rules" (so issue creation/duplicate detection runs) or
remove the firewatch failure_rules from this noop test if you only intend to
validate job setup; look for FIREWATCH_CONFIG, slack_user, and
FIREWATCH_DEFAULT_JIRA_ASSIGNEE to make the edits.
---
Nitpick comments:
In
`@ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml`:
- Around line 28-31: The pre-step currently includes a heavy cluster
provisioning chain ("chain: ipi-install") alongside "ref: ipi-conf" and "ref:
ipi-conf-aws", which makes this Slack-webhook smoke test expensive; remove or
replace "chain: ipi-install" with a lightweight/no-op pre-chain (or omit it
entirely) so the job only runs the minimal refs required, keeping "ref:
ipi-conf"/"ref: ipi-conf-aws" if they are needed for configuration or else drop
them and ensure the post step still invokes "firewatch-report-issues" as
intended; if "firewatch-report-issues" truly requires cluster context, leave
provisioning but otherwise remove "ipi-install" to make the rehearsal cheap and
fast.
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh`:
- Around line 45-47: Split the combined export and command substitution so the
exit status of cat isn't masked: assign SLACK_WEBHOOK_URL using command
substitution first (e.g., SLACK_WEBHOOK_URL=$(cat /tmp/secrets/slack/url)) and
then run export SLACK_WEBHOOK_URL separately; keep the existing file-existence
guard around the assignment (the symbols to change are SLACK_WEBHOOK_URL and the
cat /tmp/secrets/slack/url command).
🪄 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: d6e8631b-c0a6-405d-9b7a-08814b595857
📒 Files selected for processing (3)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yamlci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.shci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-ref.yaml
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
2f5b7f1 to
9bd6b5d
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. |
openshift-ci
Bot
removed
the
approved
|
@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-v2
branch
from
9bd6b5d to
7e91e6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh (1)
45-47: Optional: parameterize the Slack credential path for consistency.Other credential paths in this step (Jira token/email, private-deck creds) flow through documented env vars in
firewatch-report-issues-ref.yaml(e.g.,FIREWATCH_JIRA_API_TOKEN_PATH). Hardcoding/tmp/secrets/slack/urlworks, but introducing aFIREWATCH_SLACK_WEBHOOK_PATHenv var (default/tmp/secrets/slack/url) in the ref would make the mount path configurable and consistent with the rest of the step. Not a blocker.🤖 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 - 47, The script currently hardcodes /tmp/secrets/slack/url when exporting SLACK_WEBHOOK_URL; change it to read the path from a new env var (e.g., FIREWATCH_SLACK_WEBHOOK_PATH) with a default of /tmp/secrets/slack/url, then check that file and export SLACK_WEBHOOK_URL from it. Update the logic that references SLACK_WEBHOOK_URL to keep behavior identical but allow configuration via FIREWATCH_SLACK_WEBHOOK_PATH so the mount path becomes consistent with other variables like FIREWATCH_JIRA_API_TOKEN_PATH.
🤖 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/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh`:
- Around line 45-48: Split the export and assignment for SLACK_WEBHOOK_URL to
avoid SC2155: first read the secret into SLACK_WEBHOOK_URL (e.g. with read -r or
a safe command substitution) then trim any trailing whitespace/newline from that
variable (use parameter expansion or a small sed/xargs trim) and only after
trimming run export SLACK_WEBHOOK_URL; update the block that currently uses
export SLACK_WEBHOOK_URL=$(cat ...) accordingly.
---
Nitpick comments:
In
`@ci-operator/step-registry/firewatch/report-issues/firewatch-report-issues-commands.sh`:
- Around line 45-47: The script currently hardcodes /tmp/secrets/slack/url when
exporting SLACK_WEBHOOK_URL; change it to read the path from a new env var
(e.g., FIREWATCH_SLACK_WEBHOOK_PATH) with a default of /tmp/secrets/slack/url,
then check that file and export SLACK_WEBHOOK_URL from it. Update the logic that
references SLACK_WEBHOOK_URL to keep behavior identical but allow configuration
via FIREWATCH_SLACK_WEBHOOK_PATH so the mount path becomes consistent with other
variables like FIREWATCH_JIRA_API_TOKEN_PATH.
🪄 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: d0b9427a-f75b-4d4c-aa8d-386baad22dfa
⛔ 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 (4)
ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.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 skipped from review due to trivial changes (1)
- ci-operator/config/RedHatQE/firewatch/RedHatQE-firewatch-main__slack-webhook-test.yaml
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
92dccbd to
0e24b0b
Compare
openshift-ci
Bot
added
the
needs-rebase
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
0e24b0b to
28ee4c2
Compare
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
1 similar comment
|
@amp-rh, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
28ee4c2 to
1a1b403
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
[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 |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
1a1b403 to
18c3fb1
Compare
|
/retest |
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
18c3fb1 to
b7c3bfc
Compare
amp-rh
force-pushed
the
interop-8979/firewatch-slack-webhook-v2
branch
from
b7c3bfc to
e6707ba
Compare
|
[REHEARSALNOTIFIER]
A total of 464 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@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. |
2 participants
Summary
firewatch-slack-notify-webhook-urlsecret at/tmp/secrets/slackin thefirewatch-report-issuesstep/tmp/secrets/slack/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 keyurlTest plan
slack-webhook-testjob from this PRRelates: https://issues.redhat.com/browse/INTEROP-8979
Summary by CodeRabbit
New Features
Chores