Updated notification variables. #2171
Conversation
WalkthroughThe pull request introduces three new notification metadata environment variables (VORTEX_NOTIFY_BRANCH, VORTEX_NOTIFY_SHA, VORTEX_NOTIFY_PR_NUMBER) across deployment config, hook library, core notify script, channel-specific notify scripts, tests, manual helpers, and docs; GitHub/JIRA/NewRelic flows updated to prefer branch/sha-based values instead of label-only refs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/hooks/library/notify-deployment.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/hooks/library/notify-deployment.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (20)
.lagoon.yml(2 hunks).vortex/docs/content/workflows/notifications.mdx(1 hunks).vortex/tests/bats/unit/notify-email.bats(5 hunks).vortex/tests/bats/unit/notify-github.bats(9 hunks).vortex/tests/bats/unit/notify-jira.bats(3 hunks).vortex/tests/bats/unit/notify-newrelic.bats(3 hunks).vortex/tests/bats/unit/notify-slack.bats(9 hunks).vortex/tests/bats/unit/notify-webhook.bats(4 hunks).vortex/tests/bats/unit/notify.bats(3 hunks).vortex/tests/manual/try-email-notification.sh(1 hunks).vortex/tests/manual/try-github-notification.sh(1 hunks).vortex/tests/manual/try-jira-notification.sh(1 hunks).vortex/tests/manual/try-newrelic-notification.sh(1 hunks).vortex/tests/manual/try-slack-notification.sh(1 hunks).vortex/tests/manual/try-webhook-notification.sh(1 hunks)hooks/library/notify-deployment.sh(1 hunks)scripts/vortex/notify-github.sh(6 hunks)scripts/vortex/notify-jira.sh(2 hunks)scripts/vortex/notify-newrelic.sh(2 hunks)scripts/vortex/notify.sh(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/notify-newrelic.bats.vortex/tests/bats/unit/notify-webhook.bats.vortex/tests/bats/unit/notify-email.bats.vortex/tests/bats/unit/notify.bats.vortex/tests/bats/unit/notify-slack.bats.vortex/tests/bats/unit/notify-github.bats.vortex/tests/bats/unit/notify-jira.bats
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
scripts/vortex/notify-github.sh
🧬 Code graph analysis (2)
scripts/vortex/notify-github.sh (1)
scripts/vortex/notify.sh (2)
fail(55-55)note(51-51)
scripts/vortex/notify.sh (1)
scripts/vortex/notify-github.sh (3)
fail(46-46)info(44-44)note(42-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: build (0)
- GitHub Check: build (1)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (3)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.2)
🔇 Additional comments (27)
.vortex/tests/bats/unit/notify-webhook.bats (1)
17-18: LGTM! Deployment context variables added consistently.The addition of
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAacross all test cases provides the required deployment context for webhook notifications. The values are consistent throughout the test suite.Also applies to: 49-50, 71-72, 100-101
.vortex/docs/content/workflows/notifications.mdx (1)
39-62: Excellent documentation of deployment context variables.The new section clearly explains the purpose and usage of the deployment context variables with concrete examples for both branch and PR scenarios. The comparison table effectively illustrates the differences between deployment types.
.vortex/tests/bats/unit/notify-newrelic.bats (1)
22-23: LGTM! Consistent with other notification tests.The deployment context variables are properly initialized across all New Relic test scenarios, maintaining consistency with the webhook and other notification test suites.
Also applies to: 63-64, 92-93
.vortex/tests/bats/unit/notify.bats (1)
35-36: LGTM! General notification tests updated correctly.The deployment context variables are properly added to general notification test cases, ensuring the base notification functionality tests include the new required variables.
Also applies to: 61-62, 127-128
.vortex/tests/bats/unit/notify-email.bats (4)
16-18: LGTM! Branch deployment test updated correctly.The branch deployment test now includes the required deployment context variables with appropriate values for a branch deployment scenario.
45-48: LGTM! PR deployment test properly configured.The PR test correctly sets all three deployment context variables including
VORTEX_NOTIFY_PR_NUMBERand updates the label to follow the "PR-123" convention. This properly distinguishes PR deployments from branch deployments.
59-60: LGTM! Assertions updated to match PR label format.The test assertions correctly expect the PR label format ("PR-123") instead of the branch name, aligning with the PR deployment scenario.
76-77: LGTM! Support test cases updated consistently.Both the pre-deployment skip and shell injection protection tests include the new deployment context variables, maintaining consistency across all test scenarios.
Also applies to: 100-101
.vortex/tests/manual/try-webhook-notification.sh (1)
43-46: LGTM! Manual test scenarios properly configured.Both PR and branch deployment scenarios are correctly configured with appropriate deployment context variables. The PR scenario includes all PR-specific metadata while the branch scenario uses simpler branch-based values.
Also applies to: 50-52
.vortex/tests/manual/try-email-notification.sh (1)
35-38: LGTM! Manual email test scenarios aligned with webhook tests.The deployment context variables are consistently configured across both manual test scripts, ensuring uniform testing of PR and branch deployment notifications.
Also applies to: 42-44
.lagoon.yml (1)
99-103: Same SHA prefix verification applies to post-deployment.The same SHA stripping logic is used here (Line 101). Please apply the same verification as noted for the pre-deployment block to ensure this matches Lagoon's actual output format.
.vortex/tests/bats/unit/notify-github.bats (1)
29-30: LGTM! Test variables align with new deployment context.The addition of
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAprovides proper branch/commit context for GitHub deployment notifications. The PR-specific test correctly models PR deployments with branch, SHA, PR number, and PR-based label.Also applies to: 61-64
hooks/library/notify-deployment.sh (1)
28-30: LGTM! Clean addition of deployment context variables.The new variables provide essential deployment metadata:
VORTEX_NOTIFY_BRANCHcaptures the deployment branchVORTEX_NOTIFY_SHAextracts the commit SHA with a sensible fallback to branch nameVORTEX_NOTIFY_PR_NUMBERis appropriately empty in the Acquia Cloud hook context.vortex/tests/bats/unit/notify-jira.bats (1)
46-47: LGTM! Consistent test variable additions.All Jira notification tests now include
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHA, aligning with the broader deployment context updates across the notification system.Also applies to: 69-70, 108-109
.vortex/tests/bats/unit/notify-slack.bats (1)
46-49: LGTM! PR test correctly models PR-based deployments.The PR scenario now includes branch, SHA, and PR number context, with the expected output correctly updated to reflect "PR-123" deployment naming instead of feature branch naming.
Also applies to: 60-60
scripts/vortex/notify-jira.sh (1)
30-32: LGTM! New branch variable added for issue extraction.The addition of
VORTEX_NOTIFY_JIRA_BRANCHprovides a dedicated source for extracting Jira issue numbers, with a proper fallback chain toVORTEX_NOTIFY_BRANCH.scripts/vortex/notify-newrelic.sh (2)
31-32: LGTM! SHA variable added for accurate revision tracking.Adding
VORTEX_NOTIFY_NEWRELIC_SHAenables New Relic deployments to use actual commit SHAs for revision tracking, which is more meaningful than auto-generated timestamp-based values.
94-101: Excellent improvement: SHA-based revision with sensible fallback.The revised logic prioritizes the commit SHA when available, falling back to the previous LABEL-TIMESTAMP format for backward compatibility. This provides more accurate deployment tracking in New Relic.
.vortex/tests/manual/try-slack-notification.sh (1)
43-46: LGTM! Manual test scenarios updated correctly.Both PR and branch deployment scenarios now include the new
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAvariables, with PR scenario additionally providing PR number and PR-based label.Also applies to: 50-51
.vortex/tests/manual/try-newrelic-notification.sh (1)
47-51: LGTM! Manual test scenarios configured properly.The PR and branch scenarios correctly set the new deployment context variables. App name conventions appropriately distinguish between PR-based (
Test Project with PR-PR-123) and branch-based (Test Project-main) deployments.Also applies to: 55-58
.vortex/tests/manual/try-jira-notification.sh (1)
51-62: LGTM: Notification variables properly aligned in test scenarios.Both PR and branch scenarios correctly initialize new deployment context variables. The consistency between
VORTEX_NOTIFY_PR_NUMBER="123"andVORTEX_NOTIFY_LABEL="PR-123"is good, and the absence of PR_NUMBER in the branch scenario is semantically correct.scripts/vortex/notify-github.sh (2)
56-56: LGTM: Validation and API payloads correctly use branch reference.The validation at line 56 properly enforces the required variable, and the GitHub API calls at lines 91 and 112 correctly use
VORTEX_NOTIFY_GITHUB_BRANCHas the ref parameter, which aligns with GitHub's Deployment API expectations.Also applies to: 91-91, 112-112
25-27: The change fromVORTEX_NOTIFY_GITHUB_LABELtoVORTEX_NOTIFY_GITHUB_BRANCHis correct and safe. No references toVORTEX_NOTIFY_GITHUB_LABELexist in the codebase, confirming the migration is complete. GitHub's Deployment API correctly expects a git ref (branch/tag) rather than a label, and the fallback chain ensures backward compatibility.scripts/vortex/notify.sh (3)
30-37: LGTM: New variables properly documented and structured.The three new variables are well-documented. The distinction between required (BRANCH, SHA) and optional (PR_NUMBER) is clear from the code structure.
74-95: LGTM: Proper export and helpful summary display.Variables are correctly exported at lines 74-76 for downstream scripts, and the new summary block (lines 86-95) improves observability by displaying complete deployment context. The optional fallback for PR_NUMBER at line 90 is appropriate.
63-64: Upstream integration already in place: both required variables are properly set by callers.Lines 63-64 enforce
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAas required. Verification confirms that all known callers already set these variables before invoking notify.sh:
hooks/library/notify-deployment.shexports both from Acquia Cloud hook parameters (lines 28-29).lagoon.ymlpre-deployment and post-deployment sections export both from Lagoon environment variables (lines 28-31, 99-100)No upstream changes are needed; the contract is already enforced and satisfied.
.vortex/tests/manual/try-github-notification.sh (1)
56-62: LGTM: Manual test script aligned with new required variables.The consolidated deployment context block properly sets all required variables (
VORTEX_NOTIFY_BRANCH,VORTEX_NOTIFY_SHA), optional PR metadata, and maintains consistency with the updated notify.sh validation. Good example of proper variable usage.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2171 +/- ##
===========================================
- Coverage 93.49% 76.10% -17.39%
===========================================
Files 8 105 +97
Lines 169 5717 +5548
Branches 44 0 -44
===========================================
+ Hits 158 4351 +4193
- Misses 11 1366 +1355 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
force-pushed
the
feature/fix-github-notifications-wait-checks
branch
from
05102b7 to
6215789
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (5)
.vortex/installer/tests/Fixtures/handler_process/hosting_acquia/hooks/library/notify-deployment.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___acquia/hooks/library/notify-deployment.shis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/hosting_project_name___lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**.vortex/installer/tests/Fixtures/handler_process/provision_database_lagoon/.lagoon.ymlis excluded by!.vortex/installer/tests/Fixtures/**
📒 Files selected for processing (21)
.lagoon.yml(2 hunks).vortex/docs/content/workflows/notifications.mdx(1 hunks).vortex/docs/content/workflows/variables.mdx(3 hunks).vortex/tests/bats/unit/notify-email.bats(5 hunks).vortex/tests/bats/unit/notify-github.bats(9 hunks).vortex/tests/bats/unit/notify-jira.bats(3 hunks).vortex/tests/bats/unit/notify-newrelic.bats(3 hunks).vortex/tests/bats/unit/notify-slack.bats(9 hunks).vortex/tests/bats/unit/notify-webhook.bats(4 hunks).vortex/tests/bats/unit/notify.bats(3 hunks).vortex/tests/manual/try-email-notification.sh(1 hunks).vortex/tests/manual/try-github-notification.sh(1 hunks).vortex/tests/manual/try-jira-notification.sh(1 hunks).vortex/tests/manual/try-newrelic-notification.sh(1 hunks).vortex/tests/manual/try-slack-notification.sh(1 hunks).vortex/tests/manual/try-webhook-notification.sh(1 hunks)hooks/library/notify-deployment.sh(2 hunks)scripts/vortex/notify-github.sh(6 hunks)scripts/vortex/notify-jira.sh(2 hunks)scripts/vortex/notify-newrelic.sh(2 hunks)scripts/vortex/notify.sh(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-08T12:02:24.652Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1896
File: .vortex/tests/bats/unit/download-db-lagoon.bats:24-25
Timestamp: 2025-08-08T12:02:24.652Z
Learning: In .vortex/tests/bats/unit Bats tests using ../_helper.bash (run_steps), prefixing a STEPS entry with "- " denotes a negative assertion (the substring must NOT appear in output). Unprefixed entries are positive assertions. Example: "- Database dump refresh requested. Will create a new dump." asserts absence; "Database dump refresh requested. Will create a new dump." asserts presence.
Applied to files:
.vortex/tests/bats/unit/notify-newrelic.bats.vortex/tests/bats/unit/notify-jira.bats.vortex/tests/bats/unit/notify-webhook.bats.vortex/tests/bats/unit/notify-email.bats.vortex/tests/bats/unit/notify.bats.vortex/tests/bats/unit/notify-slack.bats.vortex/tests/bats/unit/notify-github.bats
📚 Learning: 2025-07-23T01:16:30.963Z
Learnt from: AlexSkrypnyk
Repo: drevops/vortex PR: 1816
File: .docker/cli.dockerfile:72-76
Timestamp: 2025-07-23T01:16:30.963Z
Learning: In the Vortex project, unauthenticated Composer installs should be allowed, so GitHub token secrets should not be marked as `required=true` in Docker build mounts. The conditional check `if [ -s /run/secrets/github_token ]` is the preferred approach to allow builds to proceed without a token when needed.
Applied to files:
scripts/vortex/notify-github.sh
🧬 Code graph analysis (2)
.vortex/tests/manual/try-jira-notification.sh (1)
.vortex/installer/tests/Helpers/TuiOutput.php (1)
echo(196-200)
scripts/vortex/notify-github.sh (1)
scripts/vortex/notify.sh (2)
fail(55-55)note(51-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: build (1)
- GitHub Check: build (0)
- GitHub Check: vortex-test-installer (8.4)
- GitHub Check: vortex-test-installer (8.3)
- GitHub Check: vortex-test-installer (8.2)
- GitHub Check: vortex-test-workflow (0)
- GitHub Check: vortex-test-workflow (2)
- GitHub Check: vortex-test-workflow (4)
- GitHub Check: vortex-test-workflow (1)
- GitHub Check: vortex-test-common
- GitHub Check: vortex-test-workflow (3)
🔇 Additional comments (28)
scripts/vortex/notify-newrelic.sh (1)
92-103: LGTM! Revision generation now prioritizes SHA.The updated logic correctly uses the commit SHA when available and falls back to label+timestamp for backward compatibility. This provides more accurate revision tracking in New Relic.
.vortex/tests/bats/unit/notify-webhook.bats (1)
17-18: LGTM! Test setup updated with branch/SHA context.The addition of
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAexports provides consistent deployment context across all webhook notification test scenarios.Also applies to: 49-50, 71-72, 100-101
.vortex/docs/content/workflows/notifications.mdx (1)
39-62: Excellent documentation for deployment context variables.The new section clearly explains the purpose and usage of branch/SHA/PR-number variables, with helpful examples distinguishing branch from PR deployments.
.vortex/tests/bats/unit/notify-newrelic.bats (1)
22-23: LGTM! New Relic tests updated with branch/SHA context.Consistent test setup additions align with the PR's introduction of deployment context variables.
Also applies to: 63-64, 92-93
.vortex/tests/bats/unit/notify.bats (1)
35-36: LGTM! General notification tests updated with branch/SHA context.The additions ensure all notification test scenarios have access to deployment context variables.
Also applies to: 61-62, 127-128
.lagoon.yml (1)
30-32: LGTM! Deployment context variables correctly exported.The variable assignments properly source from Lagoon environment variables:
VORTEX_NOTIFY_BRANCHuses PR head branch or falls back to git branchVORTEX_NOTIFY_SHAdirectly uses the git SHAVORTEX_NOTIFY_PR_NUMBERfalls back to empty for non-PR deploymentsNote: The past review comment about removing SHA prefix stripping appears to have been addressed, as the current code contains no such operation.
Also applies to: 98-100
.vortex/tests/bats/unit/notify-jira.bats (1)
46-47: LGTM! JIRA tests updated with branch/SHA context.The test setup additions align with the behavioral change where issue extraction now uses branch names instead of labels. The test branch names appropriately contain JIRA issue patterns for validation.
Also applies to: 69-70, 108-109
.vortex/tests/manual/try-webhook-notification.sh (1)
43-52: LGTM! Webhook test script correctly updated with new notification variables.The addition of VORTEX_NOTIFY_BRANCH, VORTEX_NOTIFY_SHA, and VORTEX_NOTIFY_PR_NUMBER (for PR scenarios) properly demonstrates the new notification metadata structure. The hardcoded test values are appropriate for manual testing.
.vortex/tests/manual/try-email-notification.sh (1)
35-44: LGTM! Email test script properly mirrors the webhook test pattern.The new notification variables are added consistently across both PR and branch scenarios.
hooks/library/notify-deployment.sh (2)
29-31: LGTM! Hook exports correctly structured notification metadata.The exports provide branch, SHA, and an empty PR number (appropriate for Acquia non-PR deployments) to downstream notification scripts.
12-12: This code correctly uses the 4th positional argument provided by Acquia Cloud hooks.Acquia Cloud post-code-deploy hooks are invoked with six positional arguments, where
$4is the deployed-tag (the tag deployed to the target environment). The code correctly assigns this to therefvariable and subsequently exports it asVORTEX_NOTIFY_SHAfor deployment notifications. No action required..vortex/tests/bats/unit/notify-slack.bats (3)
17-19: LGTM! Branch notification test properly includes new metadata variables.
46-60: LGTM! PR notification test correctly validates new PR-specific metadata.The test properly exports VORTEX_NOTIFY_BRANCH, VORTEX_NOTIFY_SHA, and VORTEX_NOTIFY_PR_NUMBER, and validates that the output reflects the PR-123 deployment naming.
77-79: LGTM! All remaining Slack test cases consistently include the new variables.The new VORTEX_NOTIFY_BRANCH and VORTEX_NOTIFY_SHA variables are properly added across pre_deployment, missing webhook, failure, custom channel, commit SHA, and shell injection test scenarios.
Also applies to: 104-106, 129-131, 154-156, 183-184, 211-213
.vortex/tests/bats/unit/notify-email.bats (3)
16-18: LGTM! Branch email notification test includes new metadata variables.
45-60: LGTM! PR email test correctly validates PR-specific naming in subject and body.The test properly verifies that both the subject line and body reflect the "PR-123" deployment naming.
76-77: LGTM! Pre-deployment and shell injection tests include new variables.Also applies to: 100-101
.vortex/tests/manual/try-jira-notification.sh (1)
51-60: LGTM! JIRA test script properly updated with new notification variables.The branch name pattern
feature/${JIRA_ISSUE}-test-notificationis appropriate for testing JIRA notifications, as it allows the notification system to extract the JIRA issue reference from the branch name..vortex/tests/manual/try-newrelic-notification.sh (1)
47-58: LGTM! New Relic test script properly updated with new notification variables.The NEWRELIC_APP_NAME defaults appropriately reflect the PR vs branch context.
.vortex/tests/manual/try-slack-notification.sh (1)
43-52: LGTM! Slack test script properly updated with new notification variables.The changes are consistent with the other manual test scripts reviewed.
.vortex/tests/bats/unit/notify-github.bats (1)
29-30: LGTM: Test variable additions are consistent.The new exports for
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAare consistently added across all test scenarios, ensuring uniform test coverage for the new notification metadata.Also applies to: 62-62, 95-95, 125-125, 157-158, 190-191, 221-222, 253-254
scripts/vortex/notify-github.sh (2)
25-27: LGTM: Clear variable naming and documentation.The rename from
VORTEX_NOTIFY_GITHUB_LABELtoVORTEX_NOTIFY_GITHUB_BRANCHbetter reflects the actual usage as a Git ref in the GitHub Deployments API. The comment clearly explains its purpose, and the default value provides a good fallback.
56-56: LGTM: Validation updated correctly.The validation logic properly checks for the renamed
VORTEX_NOTIFY_GITHUB_BRANCHvariable, ensuring consistency with the variable name change..vortex/tests/manual/try-github-notification.sh (1)
56-62: LGTM: Consolidated setup improves clarity.The centralized setup block clearly defines all deployment context variables in one place, making the test script easier to understand and maintain.
scripts/vortex/notify.sh (4)
30-40: LGTM: Clear variable definitions and purpose.The new deployment metadata variables are well-documented with clear comments explaining their purpose. The distinction between technical identifiers (BRANCH, SHA, PR_NUMBER) and display label (LABEL) is well articulated.
63-64: LGTM: Appropriate validation for required metadata.The validation correctly enforces that
VORTEX_NOTIFY_BRANCHandVORTEX_NOTIFY_SHAare required, while keepingVORTEX_NOTIFY_PR_NUMBERoptional. This makes sense as not all deployments are PR-based.
74-76: LGTM: Proper variable exports for downstream scripts.The new variables are correctly exported so channel-specific notification scripts (GitHub, Slack, etc.) can access the deployment metadata.
86-95: LGTM: Comprehensive notification summary.The expanded notification summary provides clear visibility into all deployment metadata, including the optional PR number with a helpful
<none>fallback.
| | <a id="vortex_lagooncli_path"></a>`VORTEX_LAGOONCLI_PATH` | Location of the Lagoon CLI binary. | `/tmp` | `scripts/vortex/deploy-lagoon.sh`, `scripts/vortex/task-custom-lagoon.sh` | | ||
| | <a id="vortex_lagooncli_version"></a>`VORTEX_LAGOONCLI_VERSION` | Lagoon CLI version to use. | `v0.32.0` | `scripts/vortex/deploy-lagoon.sh`, `scripts/vortex/task-custom-lagoon.sh` | | ||
| | <a id="vortex_lagoon_production_branch"></a>`VORTEX_LAGOON_PRODUCTION_BRANCH` | Dedicated branch to identify the production environment. | `main` | `.env` | | ||
| | <a id="vortex_notify_branch"></a>`VORTEX_NOTIFY_BRANCH` | Notification git branch name. | `UNDEFINED` | `scripts/vortex/notify.sh` | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify expected format and source for VORTEX_NOTIFY_BRANCH.
State whether this should be a plain branch name (e.g., feature/foo) vs a full ref, and note the typical auto-detection source in CI (e.g., GITHUB_HEAD_REF or CI_COMMIT_BRANCH). Also mention it seeds channel-specific branch vars as the canonical default.
🤖 Prompt for AI Agents
.vortex/docs/content/workflows/variables.mdx around line 260: the docs entry for
VORTEX_NOTIFY_BRANCH needs to explicitly state the expected value format and
typical CI source. Update the description cell to say this should be a plain
branch name (e.g., feature/foo), not a full ref (no refs/heads/...), list common
CI auto-detection environment vars to check (e.g., GITHUB_HEAD_REF for PRs,
CI_COMMIT_BRANCH or GITHUB_REF_NAME for branch builds), and note that this
variable is used to seed channel-specific branch vars as the canonical default;
keep the default and usage examples intact.
| | <a id="vortex_notify_email_recipients"></a>`VORTEX_NOTIFY_EMAIL_RECIPIENTS` | Email address(es) to send notifications to.<br/><br/>Applies to email notifications.<br/><br/>Multiple names can be specified as a comma-separated list of email addresses with optional names in the format "email|name". Example: "to1@example.com|Jane Doe, to2@example.com|John Doe" | `webmaster@your-site-domain.example|Webmaster` | `.env`, `scripts/vortex/notify-email.sh` | | ||
| | <a id="vortex_notify_environment_url"></a>`VORTEX_NOTIFY_ENVIRONMENT_URL` | Notification environment URL (where the site was deployed). | `UNDEFINED` | `scripts/vortex/notify.sh` | | ||
| | <a id="vortex_notify_event"></a>`VORTEX_NOTIFY_EVENT` | Notification event type.<br/><br/>Can be 'pre_deployment' or 'post_deployment'. | `post_deployment` | `scripts/vortex/notify.sh` | | ||
| | <a id="vortex_notify_github_branch"></a>`VORTEX_NOTIFY_GITHUB_BRANCH` | GitHub notification git branch name. This will be used as the 'ref' parameter in GitHub's Deployment API. | `${VORTEX_NOTIFY_BRANCH}` | `scripts/vortex/notify-github.sh` | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .vortex/docs/content/workflows/variables.mdx | sed -n '270,280p'Repository: drevops/vortex
Length of output: 19739
🌐 Web query:
GitHub Deployments API ref parameter requirements documentation
💡 Result:
Short answer: The deployments API requires the ref parameter (when creating) and it must be a branch name, tag name, or a commit SHA (short or long). See GitHub docs and Octokit reference. [1][2]
Sources:
[1] GitHub REST docs — Create a deployment: "The ref parameter can be any named branch, tag, or SHA."
[2] Octokit / deployments reference — "ref: The ref to deploy. This can be a branch, tag, or SHA" (short or long SHA supported).
GitHub ref semantics: clarify allowed types; align case sensitivity in environment documentation.
Line 272 should document that GitHub Deployments API's ref parameter must be an existing branch name, tag, or commit SHA. Line 273 lists pr in lowercase within allowed values but defaults to PR (uppercase)—clarify which case is correct, or adjust the default/description to match.
🤖 Prompt for AI Agents
.vortex/docs/content/workflows/variables.mdx around line 272: the description
for VORTEX_NOTIFY_GITHUB_BRANCH must clarify that GitHub Deployments API's ref
parameter accepts an existing branch name, tag, or commit SHA, and there is a
case mismatch between the allowed value list and the default for the PR option;
update the doc text to explicitly state "branch name, tag, or commit SHA" as
allowed ref types and make the PR value casing consistent by either changing the
listed allowed value to "PR" or changing the default from `PR` to `pr` so both
the allowed-values list and the default match exactly.
| | <a id="vortex_notify_github_repository"></a>`VORTEX_NOTIFY_GITHUB_REPOSITORY` | GitHub notification repository in owner/repo format. | `UNDEFINED` | `scripts/vortex/notify-github.sh` | | ||
| | <a id="vortex_notify_github_token"></a>`VORTEX_NOTIFY_GITHUB_TOKEN` | GitHub notification personal access token. | `${GITHUB_TOKEN}` | `scripts/vortex/notify-github.sh`, `ACQUIA ENVIRONMENT`, `LAGOON ENVIRONMENT` | | ||
| | <a id="vortex_notify_jira_assignee_email"></a>`VORTEX_NOTIFY_JIRA_ASSIGNEE_EMAIL` | JIRA notification assignee email address.<br/><br/>If left empty - no assignment will be performed. | `UNDEFINED` | `scripts/vortex/notify-jira.sh` | | ||
| | <a id="vortex_notify_jira_branch"></a>`VORTEX_NOTIFY_JIRA_BRANCH` | JIRA notification git branch name (used for issue extraction). | `${VORTEX_NOTIFY_BRANCH}` | `scripts/vortex/notify-jira.sh` | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Describe JIRA branch parsing expectations.
Add one sentence on the expected issue-key pattern extraction (e.g., PROJ-123 in the branch name), whether multiple keys are supported, and what happens if none are found.
🤖 Prompt for AI Agents
In .vortex/docs/content/workflows/variables.mdx around line 279, add one concise
sentence describing branch-to-issue-key parsing: specify the exact regex/format
used to extract issue keys (e.g., uppercase project key, hyphen, digits like
PROJ-123), state whether the parser extracts multiple keys from a branch name
(and how they are separated/handled) and explain the behavior when no keys are
found (e.g., no JIRA notifications are sent and the script logs a warning or
exits gracefully). Ensure the sentence uses the same tense/style as the
surrounding table.
| | <a id="vortex_notify_jira_environment_url"></a>`VORTEX_NOTIFY_JIRA_ENVIRONMENT_URL` | JIRA notification environment URL. | `${VORTEX_NOTIFY_ENVIRONMENT_URL}` | `scripts/vortex/notify-jira.sh` | | ||
| | <a id="vortex_notify_jira_event"></a>`VORTEX_NOTIFY_JIRA_EVENT` | JIRA notification event type. Can be 'pre_deployment' or 'post_deployment'. | `post_deployment` | `scripts/vortex/notify-jira.sh` | | ||
| | <a id="vortex_notify_jira_label"></a>`VORTEX_NOTIFY_JIRA_LABEL` | JIRA notification deployment label (branch name, PR number, or custom identifier). | `${VORTEX_NOTIFY_LABEL}` | `scripts/vortex/notify-jira.sh` | | ||
| | <a id="vortex_notify_jira_label"></a>`VORTEX_NOTIFY_JIRA_LABEL` | JIRA notification deployment label (human-readable identifier for display). | `${VORTEX_NOTIFY_LABEL}` | `scripts/vortex/notify-jira.sh` | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reinforce “display-only” semantics for JIRA label.
Explicitly state this label is not used for issue extraction or transitions; it’s purely presentation. Helps prevent misconfiguration.
🤖 Prompt for AI Agents
.vortex/docs/content/workflows/variables.mdx around line 283: the
VORTEX_NOTIFY_JIRA_LABEL entry currently only calls it a "JIRA notification
deployment label" but doesn't state it is display-only; update the description
to explicitly state this variable is used solely for human-readable
display/presentation and is not used for issue extraction, status transitions,
or automation logic, and add a brief example or note clarifying it must not be
relied upon for programmatic processing.
| | <a id="vortex_notify_newrelic_user_key"></a>`VORTEX_NOTIFY_NEWRELIC_USER_KEY` | New Relic notification User API Key.<br/><br/>To obtain your User API Key: `1`. Log in to New Relic `2`. Click on your profile icon (bottom left) `3`. Go to "API keys" `4`. Create or copy an existing "User key" `5`. The key format is: NRAK-XXXXXXXXXXXXXXXXXXXXXX<br/><br/>@see https://docs.newrelic.com/docs/apis/intro-apis/new-relic-api-keys/#user-key @see https://www.vortextemplate.com/docs/workflows/notifications#new-relic | `${NEWRELIC_USER_KEY}` | `scripts/vortex/notify-newrelic.sh` | | ||
| | <a id="vortex_notify_project"></a>`VORTEX_NOTIFY_PROJECT` | Notification project name. | `${VORTEX_PROJECT}` | `scripts/vortex/notify.sh` | | ||
| | <a id="vortex_notify_pr_number"></a>`VORTEX_NOTIFY_PR_NUMBER` | Notification pull request number. | `UNDEFINED` | `scripts/vortex/notify.sh` | | ||
| | <a id="vortex_notify_sha"></a>`VORTEX_NOTIFY_SHA` | Notification git commit SHA. | `UNDEFINED` | `scripts/vortex/notify.sh` | |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Define SHA format and relationship to branch for VORTEX_NOTIFY_SHA.
Document that this should be the full 40-char commit SHA corresponding to the deployed ref and is preferred by channels that support immutable references.
🤖 Prompt for AI Agents
.vortex/docs/content/workflows/variables.mdx around line 309: update the
VORTEX_NOTIFY_SHA variable documentation to state that it must be the full
40-character Git commit SHA corresponding to the deployed ref, clarify it
represents the exact commit for the deployed branch/ref, and note that channels
which support immutable references prefer this full SHA over short SHAs or
branch names; ensure the wording explicitly mentions "full 40‑char commit SHA"
and the relationship to the deployed ref (i.e., it must match the commit that
was deployed).
Related to #1923
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.