-
-
Notifications
You must be signed in to change notification settings - Fork 21
Updated client config controller to reference updated script name #1330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Updated client config controller to reference updated script name #1330
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdated the backend-driven frontend client identifier from Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
ref https://linear.app/ghost/issue/PROD-2455 ref TryGhost/Ghost#24985 The `admin-x-activitypub` script has been renamed to just `activitypub` so the client config endpoint should be updated to reflect this change
2e37743
to
09f052a
Compare
9195985
to
a57802f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-pr.yml
(2 hunks)
⏰ 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). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 0' config.yml) | ||
export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guard against negative route priorities
MIN_PRIORITY
falls back to 0
, so the first time we run without any existing rules we compute NEXT_PRIORITY=-1
. Cloud Load Balancer rejects negative priorities (valid range is 0..2147483647
), so the import will fail and the workflow breaks. Please clamp the fallback and ensure we never subtract below zero.
- export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 0' config.yml)
- export NEXT_PRIORITY=$((MIN_PRIORITY - 1))
+ export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 1' config.yml)
+ if [ "$MIN_PRIORITY" -le 0 ]; then
+ export NEXT_PRIORITY=0
+ else
+ export NEXT_PRIORITY=$((MIN_PRIORITY - 1))
+ fi
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 0' config.yml) | |
export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) | |
export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 1' config.yml) | |
if [ "$MIN_PRIORITY" -le 0 ]; then | |
export NEXT_PRIORITY=0 | |
else | |
export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) | |
fi |
🤖 Prompt for AI Agents
In .github/workflows/deploy-pr.yml around lines 133-134, the script computes
NEXT_PRIORITY as MIN_PRIORITY - 1 which can produce -1 when MIN_PRIORITY falls
back to 0; Cloud Load Balancer rejects negative priorities. Change the logic so
MIN_PRIORITY is treated as unsigned and clamp NEXT_PRIORITY to never go below 0
(e.g. if MIN_PRIORITY is 0 or missing, set NEXT_PRIORITY to 0; otherwise set
NEXT_PRIORITY to MIN_PRIORITY - 1).
a57802f
to
2b31903
Compare
2b31903
to
3874095
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/deploy-pr.yml (1)
133-134
: Fix negative/invalid route priorities (duplicate of earlier review).NEXT_PRIORITY can become negative when no rules or when min=0. Clamp to non‑negative as previously noted.
Apply one of these minimal fixes:
- export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 0' config.yml) - export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) + export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 1' config.yml) + if [ "$MIN_PRIORITY" -le 0 ]; then + export NEXT_PRIORITY=0 + else + export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-pr.yml
(2 hunks)
⏰ 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). (2)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
.github/workflows/deploy-pr.yml (3)
105-112
: Staging project scoping looks correct.Using GCP_PROJECT=ghost-activitypub-stg for test DB cleanup is consistent and safe here.
117-117
: Consistent GCP project for LB update.Good: the URL‑map step now also derives project scope from GCP_PROJECT=ghost-activitypub-stg.
125-126
: Good: backend service URLs parameterized by project.DEFAULT_SERVICE and PR_SERVICE now derive from ${GCP_PROJECT}. Verify the backend service names exist in that project/region.
.github/workflows/deploy-pr.yml
Outdated
for LABEL in $(echo "$LABELS_JSON" | jq -r '.[]'); do | ||
echo "Adding route for label: $LABEL" | ||
yq -i '.pathMatchers[0].routeRules += [{"priority": '$NEXT_PRIORITY', "matchRules": [{"prefixMatch": "/", "headerMatches": [{ "headerName": "X-Forwarded-Host", "exactMatch": "'$LABEL'" }]}], "routeAction": {"weightedBackendServices": [ { "backendService": "'$PR_SERVICE'", "weight": 100 } ] }, "headerAction": { "requestHeadersToAdd": [ { "headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}" }, { "headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}" } ] } }]' config.yml | ||
yq -i '.pathMatchers[0].routeRules = [{"priority": ${NEXT_PRIORITY}, "matchRules": [{"prefixMatch": "/", "headerMatches": [{"headerName": "X-Forwarded-Host", "exactMatch": ${LABEL}}]}], "routeAction": {"weightedBackendServices": [{"backendService": ${PR_SERVICE}, "weight": 100}]}, "headerAction": {"requestHeadersToAdd": [{"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"}]}}] + .pathMatchers[0].routeRules' config.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking: yq command won’t expand ${...} due to single quotes; LABEL isn’t exported.
The single‑quoted yq program prevents shell expansion of ${NEXT_PRIORITY}, ${LABEL}, ${PR_SERVICE}, causing a parse error and empty values. Pass vars via env and use yq env/strenv.
Apply this diff:
- yq -i '.pathMatchers[0].routeRules = [{"priority": ${NEXT_PRIORITY}, "matchRules": [{"prefixMatch": "/", "headerMatches": [{"headerName": "X-Forwarded-Host", "exactMatch": ${LABEL}}]}], "routeAction": {"weightedBackendServices": [{"backendService": ${PR_SERVICE}, "weight": 100}]}, "headerAction": {"requestHeadersToAdd": [{"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"}]}}] + .pathMatchers[0].routeRules' config.yml
+ LABEL="$LABEL" yq -i '.pathMatchers[0].routeRules = ([{
+ "priority": (env(NEXT_PRIORITY) | tonumber),
+ "matchRules": [{
+ "prefixMatch": "/",
+ "headerMatches": [{
+ "headerName": "X-Forwarded-Host",
+ "exactMatch": strenv(LABEL)
+ }]
+ }],
+ "routeAction": {
+ "weightedBackendServices": [{
+ "backendService": strenv(PR_SERVICE),
+ "weight": 100
+ }]
+ },
+ "headerAction": {
+ "requestHeadersToAdd": [
+ {"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"},
+ {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"}
+ ]
+ }
+ }] + .pathMatchers[0].routeRules)' config.yml
Notes:
- Prefix LABEL=... to export it for this command.
- Use env(NEXT_PRIORITY)|tonumber for numeric priority.
- Use strenv(...) for strings to avoid YAML parsing surprises.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
yq -i '.pathMatchers[0].routeRules = [{"priority": ${NEXT_PRIORITY}, "matchRules": [{"prefixMatch": "/", "headerMatches": [{"headerName": "X-Forwarded-Host", "exactMatch": ${LABEL}}]}], "routeAction": {"weightedBackendServices": [{"backendService": ${PR_SERVICE}, "weight": 100}]}, "headerAction": {"requestHeadersToAdd": [{"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"}]}}] + .pathMatchers[0].routeRules' config.yml | |
LABEL="$LABEL" yq -i '.pathMatchers[0].routeRules = ([{ | |
"priority": (env(NEXT_PRIORITY) | tonumber), | |
"matchRules": [{ | |
"prefixMatch": "/", | |
"headerMatches": [{ | |
"headerName": "X-Forwarded-Host", | |
"exactMatch": strenv(LABEL) | |
}] | |
}], | |
"routeAction": { | |
"weightedBackendServices": [{ | |
"backendService": strenv(PR_SERVICE), | |
"weight": 100 | |
}] | |
}, | |
"headerAction": { | |
"requestHeadersToAdd": [ | |
{"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, | |
{"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"} | |
] | |
} | |
}] + .pathMatchers[0].routeRules)' config.yml |
🤖 Prompt for AI Agents
.github/workflows/deploy-pr.yml around line 138: the current yq invocation is
single-quoted so shell variables (${NEXT_PRIORITY}, ${LABEL}, ${PR_SERVICE}) are
not expanded and LABEL is not exported; fix by exporting LABEL before the
command (prefix LABEL=...), and rewrite the yq expression to use yq's env() and
strenv() functions (e.g. env(NEXT_PRIORITY)|tonumber for numeric priority and
strenv(...) for string values) so variables are passed via environment rather
than relying on shell expansion and to avoid YAML parsing issues.
3874095
to
e83b100
Compare
e83b100
to
7b790b0
Compare
7b790b0
to
ae6e62e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/deploy-pr.yml (1)
133-140
: Fix priority math, avoid negative/duplicate priorities, remove brittle yq quoting, and stop assuming pathMatchers[0]
- NEXT_PRIORITY can be -1; also incrementing it per label can collide with existing min priority.
- Single-quoted yq with inline shell interpolation is brittle; pass vars via env and use yq env/strenv.
- Do not assume .pathMatchers[0] is "all-paths"; select by name.
Use label count to allocate unique priorities strictly below existing min, clamp at 0, update the matcher by name, and pass vars via env:
- export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 0' config.yml) - export NEXT_PRIORITY=$((MIN_PRIORITY - 1)) - LABELS_JSON=$(echo "$LABELS" | jq -c '[.[] | select(.name | test("\\.ghost\\.is$")) | .name]') - for LABEL in $(echo "$LABELS_JSON" | jq -r '.[]'); do - echo "Adding route for label: $LABEL" - yq -i '.pathMatchers[0].routeRules = [{"priority": '${NEXT_PRIORITY}', "matchRules": [{"prefixMatch": "/", "headerMatches": [{"headerName": "X-Forwarded-Host", "exactMatch": "'"${LABEL}"'"}]}], "routeAction": {"weightedBackendServices": [{"backendService": "'"${PR_SERVICE}"'", "weight": 100}]}, "headerAction": {"requestHeadersToAdd": [{"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"}]}}] + .pathMatchers[0].routeRules' config.yml - export NEXT_PRIORITY=$((NEXT_PRIORITY + 1)) - done + LABELS_JSON=$(echo "$LABELS" | jq -c '[.[] | select(.name | test("\\.ghost\\.is$")) | .name]') + LABEL_COUNT=$(echo "$LABELS_JSON" | jq 'length') + export MIN_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "all-paths") | .routeRules[]?.priority] | min // 1' config.yml) + if [ "$MIN_PRIORITY" -le 0 ]; then + export BASE_PRIORITY=0 + else + # Allocate a contiguous block below existing min to avoid collisions + if [ "$LABEL_COUNT" -gt 0 ]; then + export BASE_PRIORITY=$((MIN_PRIORITY - LABEL_COUNT)) + else + export BASE_PRIORITY=$((MIN_PRIORITY - 1)) + fi + if [ "$BASE_PRIORITY" -lt 0 ]; then + export BASE_PRIORITY=0 + fi + fi + i=0 + for LABEL in $(echo "$LABELS_JSON" | jq -r '.[]'); do + PRI=$((BASE_PRIORITY + i)) + echo "Adding route for label: $LABEL with priority $PRI" + LABEL="$LABEL" PRI="$PRI" yq -i ' + .pathMatchers |= map( + if .name == "all-paths" then + .routeRules = ([{ + "priority": (env(PRI) | tonumber), + "matchRules": [{ + "prefixMatch": "/", + "headerMatches": [{ + "headerName": "X-Forwarded-Host", + "exactMatch": strenv(LABEL) + }] + }], + "routeAction": { + "weightedBackendServices": [{ + "backendService": strenv(PR_SERVICE), + "weight": 100 + }] + }, + "headerAction": { + "requestHeadersToAdd": [ + {"headerName": "X-ActivityPub-PR", "headerValue": "${{ github.event.pull_request.number }}"}, + {"headerName": "X-ActivityPub-Commit", "headerValue": "${{ github.event.pull_request.head.sha }}"} + ] + } + }] + .routeRules) + else . + end + )' config.yml + i=$((i + 1)) + doneNotes:
- min // 1 avoids negative when empty; BASE_PRIORITY block guarantees unique, non-negative priorities.
- strenv(...) and env(...) remove quoting pitfalls and ensure correct types.
- Selecting by name avoids relying on index order.
🧹 Nitpick comments (1)
.github/workflows/deploy-pr.yml (1)
106-111
: Harden loop over test DB namesQuote expansions to avoid word-splitting and handle empty sets safely.
Apply:
- TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~pr_${{ github.event.pull_request.number }}_test*" --format="value(name)" --project ${GCP_PROJECT}) - for TEST_DATABASE in ${TEST_DATABASES}; do - gcloud sql databases delete ${TEST_DATABASE} --instance=stg-netherlands-activitypub --quiet --project ${GCP_PROJECT} - done + TEST_DATABASES="$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~pr_${{ github.event.pull_request.number }}_test*" --format="value(name)" --project "${GCP_PROJECT}")" + if [ -n "$TEST_DATABASES" ]; then + while IFS= read -r TEST_DATABASE; do + [ -z "$TEST_DATABASE" ] && continue + gcloud sql databases delete "$TEST_DATABASE" --instance=stg-netherlands-activitypub --quiet --project "${GCP_PROJECT}" + done <<< "$TEST_DATABASES" + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/deploy-pr.yml
(2 hunks)
⏰ 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). (2)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
.github/workflows/deploy-pr.yml (1)
125-127
: Good: service URIs now derive from GCP_PROJECTDEFAULT_SERVICE and PR_SERVICE correctly use ${GCP_PROJECT}, eliminating the project mismatch risk.
ref https://linear.app/ghost/issue/PROD-2455
ref TryGhost/Ghost#24985
The
admin-x-activitypub
script has been renamed to justactivitypub
so the client config endpoint should be updated to reflect this change