Skip to content

GHA-219 Replace Docker-based Slack notifier with Python script#125

Merged
nils-werner-sonarsource merged 5 commits intomasterfrom
ns/gha-219-slack-no-docker
Mar 23, 2026
Merged

GHA-219 Replace Docker-based Slack notifier with Python script#125
nils-werner-sonarsource merged 5 commits intomasterfrom
ns/gha-219-slack-no-docker

Conversation

@nils-werner-sonarsource
Copy link
Contributor

@nils-werner-sonarsource nils-werner-sonarsource commented Mar 23, 2026

The rtCamp/action-slack-notify Docker action fails on sonar self-hosted private runners because no Docker daemon is available. Replace it with a Python script that calls the Slack API directly via the requests library.

  • notify-slack: replace rtCamp Docker step with notify_slack.py + requirements.txt
  • notify-slack: deprecate the jobs input / empty-message fallback (warning emitted at runtime)
  • notify-slack: add test_notify_slack.py and test-notify-slack.yml workflow
  • lock-branch: build Slack message inline and delegate to notify-slack action
  • lock-branch: remove notify_slack.py and its tests (now in notify-slack)
  • test-all: include test-notify-slack workflow

The rtCamp/action-slack-notify Docker action fails on sonar-xs runners
because no Docker daemon is available. Replace it with a Python script
that calls the Slack API directly via the requests library.

- notify-slack: replace rtCamp Docker step with notify_slack.py + requirements.txt
- notify-slack: deprecate the jobs input / empty-message fallback (warning emitted at runtime)
- notify-slack: add test_notify_slack.py and test-notify-slack.yml workflow
- lock-branch: build Slack message inline and delegate to notify-slack action
- lock-branch: remove notify_slack.py and its tests (now in notify-slack)
- test-all: include test-notify-slack workflow

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@hashicorp-vault-sonar-prod
Copy link

hashicorp-vault-sonar-prod bot commented Mar 23, 2026

GHA-219

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonar-review-alpha
Copy link

Generating a summary...

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: Good architectural direction — the Docker-based action is gone and the Python script is clean. There are two real bugs in the lock-branch integration and one security-pattern violation in the README example that need fixing before merge.

🗣️ Give feedback

- lock-branch: fix literal \n in bash string — use $'\n' for real newline
- lock-branch: remove dead slack-token input (notify-slack fetches its own token)
- lock-branch: remove SLACK_TOKEN from vault secrets (no longer needed here)
- notify-slack: fix script injection in README example (use env vars for toJSON)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonar-review-alpha
Copy link

sonar-review-alpha bot commented Mar 23, 2026

Summary

Replaces Docker-based Slack notifications with a Python script calling the Slack API directly. This fixes failures on self-hosted runners without Docker daemon. The notify-slack action now focuses purely on sending messages the caller provides, rather than detecting failures. lock-branch shifts responsibility for message composition to itself, then delegates sending to notify-slack.

What reviewers should know

Start here: Review notify-slack/notify_slack.py (new script) and notify-slack/action.yml (simplified workflow) to see the core change — direct API calls via requests library instead of Docker. Then review lock-branch/action.yml to see how message building moved into the caller (the "Build Slack Message" step constructs text, color, icon inline). The deprecated path: jobs input still triggers failure parsing in notify-slack, but notify-slack now warns to use the message input instead.

Test coverage: New test-notify-slack.yml runs both integration (sends a real test notification) and unit tests. lock-branch tests no longer cover notify_slack.py (it moved out).

Backwards compat: The jobs input in notify-slack still works and builds a failure summary, but emits a deprecation warning. Callers should use the message input and build their own message string (as lock-branch now does).


  • Generate Walkthrough
  • Generate Diagram

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: Good structural improvement overall — removing the Docker dependency is the right call and the code is clean. There are two issues to fix before merge: a silent data-loss bug where the run URL is dropped from the Slack message, and a mutable @master ref that bypasses the pinning convention used everywhere else in the repo.

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: Three of the five previous issues are resolved, but the multiline GITHUB_OUTPUT bug (#2974537845) and the @master mutable ref (#2974537851) remain open. The new README example also reproduces the multiline output bug.

🗣️ Give feedback

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: Two of the three inline issues are the same runtime bug in different locations — the multi-line GITHUB_OUTPUT truncation will silently break every Slack message sent from lock-branch (the run URL link is lost from every notification). That needs fixing before merge.

🗣️ Give feedback

Sends a real Slack notification to alerts-release-github-actions to verify
the Python-based notify-slack action works end-to-end.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

Conclusion: The new commit only adds an integration test job to test-notify-slack.yml — the previously flagged bugs remain unaddressed.

🗣️ Give feedback

- Use heredoc syntax for multi-line message output to GITHUB_OUTPUT;
  plain `echo "message=..."` truncates at the first newline, dropping
  the run URL from every Slack notification
- Pin notify-slack reference to @v1 instead of @master to prevent
  future breaking changes on master from silently affecting lock-branch
- Fix README example with same multiline GITHUB_OUTPUT bug
- Correct message input's Required column (No, not Yes)
- Add continue-on-error to integration test job so transient Slack API
  failures don't block the unit-test PR gate

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sonarqubecloud
Copy link

Copy link

@sonar-review-alpha sonar-review-alpha bot left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

🗣️ Give feedback

@nils-werner-sonarsource nils-werner-sonarsource merged commit f0cf1e1 into master Mar 23, 2026
14 checks passed
@nils-werner-sonarsource nils-werner-sonarsource deleted the ns/gha-219-slack-no-docker branch March 23, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants