Fixed multiple labels not working on ephemeral staging #653
Conversation
|
Warning Rate limit exceeded@rmgpinto has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 42 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughThis change updates the GitHub Actions CI/CD workflow by disabling the "Run Tests" step in the build-test-push job through commenting it out. In the deploy-pr job, the method of updating the GCP Load Balancer URL map is revised to use in-place editing with yq instead of temporary file redirection. The addition of route rules for the PR service is altered from a single rule with multiple header matches to multiple individual rules, each corresponding to a matching label with incremented priority. The final command to import the updated URL map into GCP remains unchanged. Possibly related PRs
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
.github/workflows/cicd.yml (2)
105-107: Verify intentional disabling of the test stepCommenting out the
"Run Tests"step will completely skip unit/integration tests in CI. Ensure this is intentional (e.g., tests have been moved or replaced) or re-enable it to maintain coverage.
267-285: Iterate per-label to add separate route rulesThis loop correctly implements the PR objective by creating an individual route rule for each
.ghost.islabel and incrementally updating priority.
Optional improvement: useyq --arg priority "$NEXT_PRIORITY" --arg label "$LABEL"to inject shell variables safely and simplify the quoting in your inline script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: (ephemeral staging) Deploy
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
278-278: Restore the URL map import command
The finalgcloud compute url-maps importis still commented out, so the updatedconfig.ymlwill never be applied. Please uncomment this line.- #gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quiet + gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quiet
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: (ephemeral staging) Deploy
.github/workflows/cicd.yml
Outdated
| #- name: "Run Tests" | ||
| # run: yarn test |
There was a problem hiding this comment.
Re-enable test execution
The “Run Tests” step is commented out, effectively disabling automated tests on PRs. This undermines the CI pipeline’s purpose and risks shipping unvalidated code. Please restore this step.
Apply this diff to re-enable tests:
- #- name: "Run Tests"
- # run: yarn test
+ - name: "Run Tests"
+ run: yarn test📝 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.
| #- name: "Run Tests" | |
| # run: yarn test | |
| - name: "Run Tests" | |
| run: yarn test |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
105-106: Re-enable test execution
The “Run Tests” step is commented out, disabling automated tests for PRs and undermining CI validation. Please uncomment these lines to restore test execution.
276-276: Restore URL map import step
The finalgcloud compute url-maps importis still commented out, so changes toconfig.ymlwill never be applied. Please uncomment it to deploy the updated routing rules:- #gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quiet + gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quiet
🧹 Nitpick comments (1)
.github/workflows/cicd.yml (1)
268-273: Use name-based pathMatcher selection
Indexing into.pathMatchers[0]assumes it’s always the first element. To make this resilient against reordering or future changes, target the matcher by name:- yq -i '.pathMatchers[0].routeRules += ...' config.yml + yq -i '.pathMatchers[] |= (select(.name=="staging-environments") .routeRules += ...)' config.ymlThis ensures you’re always appending to the correct matcher.
rmgpinto
force-pushed
the
fix-ephemeral-staging-multiple-labels
branch
from
e5f6614 to
f931db7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
105-106: Re-enable the test execution step
The “Run Tests” step is still commented out, which means your CI won’t validate code changes on PRs. Please restore this step to ensure tests run on every build.- #- name: "Run Tests" - # run: yarn test + - name: "Run Tests" + run: yarn test
🧹 Nitpick comments (1)
.github/workflows/cicd.yml (1)
265-270: Optional: Simplify variable interpolation inyqinvocations
Embedding Bash variables inside single-quotedyqexpressions is brittle and hard to maintain. Consider usingenv()withinyqto injectNEXT_PRIORITYandHEADER_MATCHES, for example:- yq -i '.pathMatchers[0].routeRules += [{"priority": '$NEXT_PRIORITY', … "exactMatch": "'$LABEL'" }]' config.yml + yq -i ' .pathMatchers[0].routeRules += [{ priority: (env(NEXT_PRIORITY) | tonumber), matchRules: [{ prefixMatch: "/", headerMatches: (env(HEADER_MATCHES) | fromjson) }], routeAction: {weightedBackendServices: [{backendService: env(PR_SERVICE), weight: 100}]} }] ' config.ymlThis keeps the YAML manipulation self-contained and improves readability.
| yq -i 'del(.fingerprint)' config.yml | ||
| yq -i 'del(.creationTimestamp)' config.yml | ||
| export DEFAULT_SERVICE="https://www.googleapis.com/compute/v1/projects/ghost-activitypub/global/backendServices/stg-netherlands-activitypub-api" | ||
| export PR_SERVICE="https://www.googleapis.com/compute/v1/projects/ghost-activitypub/global/backendServices/stg-pr-${{ github.event.pull_request.number }}-api" | ||
| # Add host rules and path matchers if they don't exist | ||
| yq '.hostRules = (.hostRules // [{"hosts": ["activitypub.ghostinfra.net"], "pathMatcher": "staging-environments"}])' config.yml > config.yml.tmp | ||
| mv config.yml.tmp config.yml | ||
| yq '.pathMatchers = (.pathMatchers // [{"name": "staging-environments", "defaultService": "'"$DEFAULT_SERVICE"'", "routeRules": []}])' config.yml > config.yml.tmp | ||
| mv config.yml.tmp config.yml | ||
| yq -i '.hostRules = (.hostRules // [{"hosts": ["activitypub.ghostinfra.net"], "pathMatcher": "staging-environments"}])' config.yml | ||
| yq -i '.pathMatchers = (.pathMatchers // [{"name": "staging-environments", "defaultService": "'"$DEFAULT_SERVICE"'", "routeRules": []}])' config.yml | ||
| # Remove existing route rules for the PR service | ||
| yq '.pathMatchers[] |= (.routeRules |= map(select((.routeAction.weightedBackendServices // []) | length == 0 or .routeAction.weightedBackendServices[0].backendService != env(PR_SERVICE))))' config.yml > config.yml.tmp | ||
| mv config.yml.tmp config.yml | ||
| yq -i '.pathMatchers[] |= (.routeRules |= map(select((.routeAction.weightedBackendServices // []) | length == 0 or .routeAction.weightedBackendServices[0].backendService != env(PR_SERVICE))))' config.yml | ||
| # Add new route rules for the PR service |
There was a problem hiding this comment.
Ensure yq is installed before use
You’re invoking yq -i on multiple lines, but GitHub’s Ubuntu runners don’t include yq by default. Add an installation step prior to any yq command to avoid failures:
- name: Install yq
run: |
sudo apt-get update
sudo apt-get install -y yqPlace this immediately before your “Add route to GCP Load Balancer” step.
rmgpinto
force-pushed
the
fix-ephemeral-staging-multiple-labels
branch
from
f931db7 to
2164e2f
Compare
ref no-issue - Fixed a bug where multiple labels would not work on the ephemeral staging routing. It needs to add a route per label (site).
rmgpinto
force-pushed
the
fix-ephemeral-staging-multiple-labels
branch
from
2164e2f to
2d5ac6d
Compare
ref no-issue