Added staging tests to CI/CD #687
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 10 minutes and 25 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 (1)
WalkthroughThe Possibly related PRs
Suggested reviewers
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.7)
.github/workflows/cicd.yml
244-244: property "region" is not defined in object type {}
(expression)
248-248: property "region" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build, Test and Push
- GitHub Check: Build, Test and Push
| - name: "Deploy Tests to Cloud Run" | ||
| if: ${{ matrix.region == 'europe-west4' }} | ||
| uses: google-github-actions/deploy-cloudrun@v2 | ||
| with: | ||
| image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} | ||
| region: ${{ matrix.region }} | ||
| job: stg-pr-${{ github.event.pull_request.number }}-tests | ||
| flags: --command="yarn _test:single" --wait --execute-now | ||
| skip_default_labels: true | ||
| labels: |- | ||
| commit-sha=${{ github.sha }} | ||
There was a problem hiding this comment.
Critical: matrix.region undefined in deploy-pr job, test step never runs & wrong image tag
In the deploy-pr job, there is no strategy.matrix.region, so:
if: ${{ matrix.region == 'europe-west4' }}
always evaluates to false and the "Deploy Tests to Cloud Run" step will never execute. Additionally, the image is incorrectly using the migrations_docker_version output instead of activitypub_docker_version, so even if it ran it would deploy the wrong container.
Apply this diff:
@@ -243,7 +243,7 @@
- name: "Deploy Tests to Cloud Run"
- if: ${{ matrix.region == 'europe-west4' }}
+ # Only run tests when ephemeral staging is enabled
+ if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }}
uses: google-github-actions/deploy-cloudrun@v2
with:
- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }}
+ image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
region: europe-west4
job: stg-pr-${{ github.event.pull_request.number }}-tests
flags: --command="yarn _test:single" --wait --execute-nowCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 actionlint (1.7.7)
244-244: property "region" is not defined in object type {}
(expression)
248-248: property "region" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
In .github/workflows/cicd.yml around lines 243 to 254, the "Deploy Tests to
Cloud Run" step uses an undefined matrix.region in its if condition, causing it
to never run, and it references the wrong image tag output. Fix this by removing
or correcting the if condition to ensure the step runs as intended, and update
the image tag to use the correct output variable activitypub_docker_version
instead of migrations_docker_version.
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
402f931 to
b1dfabf
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
b1dfabf to
3ce9e17
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
3ce9e17 to
70bba90
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
70bba90 to
d9c3583
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
d9c3583 to
33a933f
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
33a933f to
d20d4f6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/test/db.ts (1)
69-79: 🛠️ Refactor suggestionUpdate cleanup connection to use socket path if available
The cleanup connection in the
afterAllhook doesn't use the socket path connection option that was added to the other connections. For consistency, consider updating it to use the same connection logic.afterAll(async () => { await dbClient.destroy(); const systemClient = knex({ client: 'mysql2', - connection: { - host: process.env.MYSQL_HOST, - port: Number.parseInt(process.env.MYSQL_PORT!), - user: process.env.MYSQL_USER, - password: process.env.MYSQL_PASSWORD, - database: 'mysql', - timezone: '+00:00', - }, + connection: process.env.MYSQL_SOCKET_PATH + ? { + socketPath: process.env.MYSQL_SOCKET_PATH, + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + } + : { + host: process.env.MYSQL_HOST, + port: Number.parseInt(process.env.MYSQL_PORT!), + user: process.env.MYSQL_USER, + password: process.env.MYSQL_PASSWORD, + database: 'mysql', + timezone: '+00:00', + }, }); await systemClient.raw(`DROP DATABASE ${dbName}`); await systemClient.destroy(); });If you implement the
getDbConnectionConfigfunction suggested earlier, you could also reuse it here with a parameter to override the database name:const systemClient = knex({ client: 'mysql2', connection: { ...getDbConnectionConfig(), database: 'mysql' }, });
🧹 Nitpick comments (1)
src/test/db.ts (1)
12-27: Enhance connection configuration by applying DRY principlesThe connection configuration is duplicated for both the
systemClientanddbClientobjects (lines 12-27 and 49-64). Consider extracting this logic into a reusable function to improve maintainability.You could create a function that returns the appropriate connection configuration:
function getDbConnectionConfig() { return process.env.MYSQL_SOCKET_PATH ? { socketPath: process.env.MYSQL_SOCKET_PATH, user: process.env.MYSQL_USER, password: process.env.MYSQL_PASSWORD, database: process.env.MYSQL_DATABASE, timezone: '+00:00', } : { host: process.env.MYSQL_HOST, port: Number.parseInt(process.env.MYSQL_PORT!), user: process.env.MYSQL_USER, password: process.env.MYSQL_PASSWORD, database: process.env.MYSQL_DATABASE, timezone: '+00:00', }; }Then use it in both places:
const systemClient = knex({ client: 'mysql2', connection: getDbConnectionConfig(), }); // And later const dbClient = knex({ client: 'mysql2', connection: getDbConnectionConfig(), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cicd.yml(3 hunks)Dockerfile(1 hunks)src/test/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- .github/workflows/cicd.yml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: (ephemeral staging) Deploy
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
d20d4f6 to
2d10476
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
243-254:⚠️ Potential issueCritical: Wrong Docker image tag in
deploy-prTests step
The "Deploy Tests to Cloud Run" step is referencing${{ needs.build-test-push.outputs.migrations_docker_version }}instead of the ActivityPub service image, so tests will run against the migrations container rather than the service under test.Please update the image to use
activitypub_docker_version:@@ -247,2 +247,2 @@ - image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
📜 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: Build, Test and Push
| - name: "Deploy Tests to Cloud Run" | ||
| if: ${{ matrix.region == 'europe-west4' }} | ||
| uses: google-github-actions/deploy-cloudrun@v2 | ||
| with: | ||
| image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} | ||
| region: ${{ matrix.region }} | ||
| job: stg-${{ matrix.region_name }}-activitypub-tests | ||
| flags: --command="yarn _test:single" --wait --execute-now | ||
| skip_default_labels: true | ||
| labels: |- | ||
| commit-sha=${{ github.sha }} | ||
There was a problem hiding this comment.
Critical: Wrong Docker image tag & inconsistent flag syntax in deploy-staging Tests step
- The image is still using
migrations_docker_versioninstead ofactivitypub_docker_version, so tests deploy the wrong container. - The
flagsinvocation differs from the PR job (using--command="yarn _test:single"vs separated--command="yarn"+--args). For clarity and consistency, split the command and args.
Fix both issues:
@@ -333,2 +333,2 @@
- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }}
+ image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
@@ -336,1 +336,2 @@
- flags: --command="yarn _test:single" --wait --execute-now
+ flags: --command="yarn" --args "_test:single" --wait --execute-now📝 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: "Deploy Tests to Cloud Run" | |
| if: ${{ matrix.region == 'europe-west4' }} | |
| uses: google-github-actions/deploy-cloudrun@v2 | |
| with: | |
| image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} | |
| region: ${{ matrix.region }} | |
| job: stg-${{ matrix.region_name }}-activitypub-tests | |
| flags: --command="yarn _test:single" --wait --execute-now | |
| skip_default_labels: true | |
| labels: |- | |
| commit-sha=${{ github.sha }} | |
| - name: "Deploy Tests to Cloud Run" | |
| if: ${{ matrix.region == 'europe-west4' }} | |
| uses: google-github-actions/deploy-cloudrun@v2 | |
| with: | |
| image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }} | |
| region: ${{ matrix.region }} | |
| job: stg-${{ matrix.region_name }}-activitypub-tests | |
| flags: --command="yarn" --args "_test:single" --wait --execute-now | |
| skip_default_labels: true | |
| labels: |- | |
| commit-sha=${{ github.sha }} |
🤖 Prompt for AI Agents
In .github/workflows/cicd.yml between lines 329 and 340, the Docker image tag is
incorrectly referencing migrations_docker_version instead of
activitypub_docker_version, causing the wrong image to deploy. Additionally, the
flags syntax is inconsistent with the PR job; it combines command and args into
a single string, which can be clearer if split into separate --command and
--args parameters. Update the image tag to use activitypub_docker_version and
split the flags into --command="yarn" and --args="_test:single" for clarity and
consistency.
closes https://linear.app/ghost/issue/PROD-1779 - Added a CI/CD step that runs tests in staging. This will attempt to catch errors in GCP components before being shipped to production.
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
c645534 to
74c931f
Compare
rmgpinto
changed the title
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
243-254: Fix Docker image tag for test deployments
The "Deploy Tests to Cloud Run" step under the PR job is still pointing at the migrations container (migrations_docker_version) instead of the ActivityPub service image. This will run tests against the wrong image.
Apply this diff:- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
329-340: Correct Docker image tag in staging tests deployment
In the staging job’s "Deploy Tests to Cloud Run" step, the image is also usingmigrations_docker_version. It should referenceactivitypub_docker_versionto run tests against the correct service image:- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
🛑 Comments failed to post (2)
.github/workflows/cicd.yml (2)
341-348:
⚠️ Potential issueAdd GCP_PROJECT env for staged database destruction
The staging cleanup step likewise uses--project ${GCP_PROJECT}without defining it. This must be set to ensure gcloud targets the correct project:- - name: "Destroy Tests databases" - if: ${{ matrix.region == 'europe-west4' }} - run: | + - name: "Destroy Tests databases" + if: ${{ matrix.region == 'europe-west4' }} + env: + GCP_PROJECT: ghost-activitypub + run: |📝 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: "Destroy Tests databases" if: ${{ matrix.region == 'europe-west4' }} env: GCP_PROJECT: ghost-activitypub run: | 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🤖 Prompt for AI Agents
In .github/workflows/cicd.yml around lines 341 to 348, the cleanup step for destroying test databases references the variable GCP_PROJECT but does not define or set it, which could cause the command to target the wrong project or fail. Define the GCP_PROJECT environment variable earlier in the workflow or within this step to specify the correct project ID, ensuring that the gcloud commands operate on the intended GCP project.
255-262:
⚠️ Potential issueDefine GCP_PROJECT for database cleanup
The cleanup step invokesgcloud sql databases delete … --project ${GCP_PROJECT}, butGCP_PROJECTisn’t defined in this step’s environment, causing it to fail. Explicitly set the project ID:- - name: "Destroy Tests databases" - if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} - run: | + - name: "Destroy Tests databases" + if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} + env: + GCP_PROJECT: ghost-activitypub + run: |📝 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: "Destroy Tests databases" if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} env: GCP_PROJECT: ghost-activitypub run: | 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🤖 Prompt for AI Agents
In .github/workflows/cicd.yml around lines 255 to 262, the cleanup step uses the variable GCP_PROJECT without defining it, which will cause the command to fail. Fix this by explicitly setting the GCP_PROJECT environment variable before running the cleanup commands, either by defining it directly in the step or referencing an existing environment variable that contains the project ID. This ensures the `gcloud` commands have the correct project context.
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
74c931f to
eacbffe
Compare
rmgpinto
force-pushed
the
add-staging-cicd-tests
branch
from
eacbffe to
bd685bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
247-247:⚠️ Potential issueCritical: Wrong Docker image tag in deploy-pr test step.
The test deployment is using
migrations_docker_versioninstead ofactivitypub_docker_version, which means it's deploying the wrong container image for tests.Apply this fix:
- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
335-335:⚠️ Potential issueCritical: Wrong Docker image tag in deploy-staging test step.
Similar to the deploy-pr job, the staging test deployment is using the wrong image tag.
Apply this fix:
- image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
🧹 Nitpick comments (2)
src/storage/gcloud-storage/gcp-storage.service.integration.test.ts (1)
8-12: Consider using a proper logger mock instead of console methods.The logger implementation directly maps to console methods, which may not be ideal for tests as it will produce output during test runs and doesn't allow for testing log behavior.
Consider using a test-specific logger mock:
-const logger = { - info: console.log, - error: console.error, - warn: console.warn, -} as unknown as import('@logtape/logtape').Logger; +const logger = { + info: vi.fn(), + error: vi.fn(), + warn: vi.fn(), +} as unknown as import('@logtape/logtape').Logger;This approach would allow you to assert on logging behavior and avoid console output during tests.
src/test/db.ts (1)
30-30: Consider simplifying the database name generation logic.The current logic is quite complex and may be difficult to understand and maintain.
Consider simplifying the database name generation:
- const dbName = `${process.env.MYSQL_DATABASE?.includes('pr-') ? `${process.env.MYSQL_DATABASE.replace(/-/g, '_')}_` : ''}test_${randomBytes(16).toString('hex')}`; + const prefix = process.env.MYSQL_DATABASE?.includes('pr-') + ? `${process.env.MYSQL_DATABASE.replace(/-/g, '_')}_` + : ''; + const dbName = `${prefix}test_${randomBytes(16).toString('hex')}`;This makes the logic more readable and easier to debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/storage/gcloud-storage/assets/dog.jpgis excluded by!**/*.jpg
📒 Files selected for processing (4)
.github/workflows/cicd.yml(2 hunks)Dockerfile(1 hunks)src/storage/gcloud-storage/gcp-storage.service.integration.test.ts(1 hunks)src/test/db.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (12)
src/storage/gcloud-storage/gcp-storage.service.integration.test.ts (7)
26-54: LGTM! Well-structured test with environment-aware assertions.The test properly handles both emulator and real GCP environments, validates URL structure, and includes appropriate assertions for different scenarios.
56-72: LGTM! Proper file size validation test.The test correctly validates the 5MB file size limit with an appropriately sized test buffer and proper error code assertion.
74-89: LGTM! Proper file type validation test.The test correctly validates unsupported file types (GIF) and asserts the expected error code.
93-113: LGTM! Good integration between saveFile and verifyImageUrl.The test properly demonstrates the integration between saving a file and then verifying its URL, with appropriate error handling.
125-137: Environment-specific error handling looks correct.The conditional error assertions based on emulator vs. real GCP environment properly handle the different error responses expected in each scenario.
139-151: Good test for non-existent file handling.The test properly uses environment variables for bucket name and handles different error codes based on the environment.
14-15:Details
✅ Verification successful
Verify the test asset file exists.
The test relies on a local image file at the specified path. Ensure this asset is included in the repository and accessible during CI runs.
🏁 Script executed:
#!/bin/bash # Description: Verify the test asset file exists and is accessible # Expected: The dog.jpg file should exist in the assets directory # Check if the test asset exists fd -t f "dog.jpg" src/storage/gcloud-storage/Length of output: 87
Test asset file verified
Thedog.jpgfile exists atsrc/storage/gcloud-storage/assets/dog.jpg, so no further action is needed..github/workflows/cicd.yml (1)
257-263: LGTM! Database cleanup step properly configured.The PR cleanup step correctly uses the GCP_PROJECT environment variable and proper variable naming as addressed from previous reviews.
src/test/db.ts (4)
12-27: LGTM! Improved connection flexibility with socket support.The conditional connection configuration properly handles both socket-based and TCP connections, improving deployment flexibility across different environments.
40-49: LGTM! Improved table cloning with constraint filtering.The enhanced table cloning logic properly handles constraints and trailing commas, which is more robust than the previous
CREATE TABLE ... LIKEapproach.
56-71: LGTM! Database client connection correctly uses test database.The connection properly uses the generated
dbNameinstead of the environment variable, addressing the previous review concern.
78-93: LGTM! Cleanup connection configuration is consistent.The afterAll cleanup properly uses the same conditional connection logic for consistency.
.github/workflows/cicd.yml
Outdated
| env: | ||
| GCP_PROJECT: ghost-activitypub | ||
| run: | | ||
| 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}) |
There was a problem hiding this comment.
Issue: Database cleanup uses wrong PR number context in staging.
The staging job cleanup step references github.event.pull_request.number, but staging jobs run on main branch pushes where this context is not available.
The staging cleanup should use a different approach since it doesn't have PR context:
- 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})
+ TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~test*" --format="value(name)" --project ${GCP_PROJECT})Or consider removing this step from staging entirely since it's meant for PR cleanup.
🤖 Prompt for AI Agents
In .github/workflows/cicd.yml at line 348, the staging cleanup step references
github.event.pull_request.number, which is not available in non-PR runs such as
staging deployments. To fix this, remove or modify this line to avoid using
PR-specific variables in staging jobs, or conditionally run this step only in PR
workflows. Ensure the cleanup logic is appropriate for the environment where it
executes.
bd685bf to
b6b9ecf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/cicd.yml (3)
343-352:⚠️ Potential issueCritical: PR-specific variable used in non-PR staging cleanup
The
deploy-stagingjob runs on pushes tomain, wheregithub.event.pull_request.numberis undefined. This makes the cleanup step fail and leaves orphaned test databases.Suggested fix:
run: | - 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}) + TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub \ + --filter="name~test*" --format="value(name)" --project ${GCP_PROJECT})Alternatively, guard this step to run only in PR workflows.
243-254:⚠️ Potential issueCritical: Incorrect Docker image tag for Tests deployment in PR job
The "Deploy Tests to Cloud Run" step is referencing the
migrations_docker_versionoutput instead ofactivitypub_docker_version, resulting in the wrong container being deployed.Suggested fix:
with: - image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
331-342:⚠️ Potential issueCritical: Incorrect Docker image tag for Tests deployment in staging job
In the
deploy-stagingjob, the "Deploy Tests to Cloud Run" step still usesmigrations_docker_versioninstead ofactivitypub_docker_version, causing an incorrect image to be deployed.Suggested fix:
with: - image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.migrations_docker_version }} + image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.build-test-push.outputs.activitypub_docker_version }}
📜 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: Build, Test and Push
| - name: "Destroy Tests databases" | ||
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | ||
| env: | ||
| GCP_PROJECT: ghost-activitypub | ||
| run: | | ||
| TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~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 | ||
There was a problem hiding this comment.
Critical: Broad database cleanup filter in PR job
The cleanup step’s filter name~test* will match all test databases, potentially deleting unrelated ones. It must target only the PR-specific databases.
Suggested fix:
run: |
- TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub \
- --filter="name~test*" --format="value(name)" --project ${GCP_PROJECT})
+ 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})📝 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: "Destroy Tests databases" | |
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | |
| env: | |
| GCP_PROJECT: ghost-activitypub | |
| run: | | |
| TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~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 | |
| - name: "Destroy Tests databases" | |
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | |
| env: | |
| GCP_PROJECT: ghost-activitypub | |
| run: | | |
| - TEST_DATABASES=$(gcloud sql databases list --instance=stg-netherlands-activitypub --filter="name~test*" --format="value(name)" --project ${GCP_PROJECT}) | |
| + 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 |
🤖 Prompt for AI Agents
In .github/workflows/cicd.yml between lines 255 and 264, the database cleanup
command uses a broad filter "name~test*" which may delete unrelated test
databases. To fix this, update the filter to target only PR-specific databases,
such as including a unique PR identifier in the database name or label, and
modify the filter accordingly to match only those databases. This ensures only
the intended ephemeral databases are deleted during cleanup.
b6b9ecf to
234f595
Compare
closes https://linear.app/ghost/issue/PROD-1779