Conversation
WalkthroughThis pull request updates the GitHub Actions workflow configuration file by renaming the workflow from "CI" to "CICD" and refining the pull_request trigger to respond only to specific actions: opened, synchronize, reopened, labeled, and unlabeled. Permissions are extended to include read access for pull-requests. The build-test-push job's environment is renamed from "staging" to "build", and steps involving GCP authentication, Artifact Registry login, and Docker image pushing are conditionally executed only for pull_request events with the specified actions. A new deploy-pr job is added to deploy ephemeral staging environments when a pull request label matches the pattern "*.ghost.is". This job performs label checking, checks out two repositories, sets up Terraform with a version read from a file, modifies Terraform module URLs and backend prefixes, authenticates with GCP, runs Terraform init and apply, deploys migrations to Cloud Run, and updates a GCP load balancer URL map to route traffic to the ephemeral environment. The deploy-staging and deploy-production jobs remain but have their environment declarations removed. Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
45dd712 to
aafd700
Compare
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
2 times, most recently
from
57a967b to
037b8fc
Compare
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
037b8fc to
07388f2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
.github/workflows/deploy.yml (1)
97-103:⚠️ Potential issueCritical Issue: Undefined Output in Deploy-Old for ActivityPub API
Finally, the "Deploy ActivityPub API to Cloud Run" step in the deploy-old job references the same undefined output. To ensure proper image deployment, update this reference to correctly obtain the intended version information from the CI workflow.
🧰 Tools
🪛 actionlint (1.7.4)
100-100: property "build-test-push" is not defined in object type {}
(expression)
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
10-33: Deploy Ephemeral Staging: Variable Usage & Environment NamingThe ephemeral staging job properly handles Google Cloud authentication and checks if the infrastructure is already provisioned. Two points to consider:
- Variable Availability: The command uses
GITHUB_HEAD_REFto filter services. Sinceworkflow_runcontexts may not always populate this variable (which is more typical in pull request events), please verify its availability or consider an alternative identifier.- Environment Naming: The job is configured with
environment: build. In the context of ephemeral staging, a more descriptive name (e.g.,ephemeral-staging) might improve clarity in your deployment logs and reports.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(1 hunks).github/workflows/deploy.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/deploy.yml
52-52: property "build-test-push" is not defined in object type {}
(expression)
60-60: property "build-test-push" is not defined in object type {}
(expression)
67-67: property "build-test-push" is not defined in object type {}
(expression)
85-85: property "build-test-push" is not defined in object type {}
(expression)
93-93: property "build-test-push" is not defined in object type {}
(expression)
100-100: property "build-test-push" is not defined in object type {}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (2)
.github/workflows/build.yml (1)
6-11: Enhanced Pull Request Event TriggersThe addition of specific pull request event types (
opened,synchronize,reopened,labeled, andunlabeled) improves the granularity of workflow triggers for PR events. This should help ensure that changes are properly tested during development..github/workflows/deploy.yml (1)
1-8: Solid Setup for the New Deployment WorkflowThe workflow is clearly set up to trigger on
workflow_runfor the "CI" workflow with thecompletedtype. This ensures that the deployment process only begins after the CI process finishes. It would be beneficial to document in your repository how the outputs from the CI workflow (like the image version tags) are expected to be passed along, as they are referenced later in the file.
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
07388f2 to
285f845
Compare
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
285f845 to
d6970c0
Compare
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
d6970c0 to
623a891
Compare
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: 1
🔭 Outside diff range comments (1)
.github/workflows/deploy.yml (1)
85-117:⚠️ Potential issueDeploy-Old Job: Missing Dependency Declaration
Similarly, the "deploy-old" job uses outputs from theci-docker-tagsjob without declaring a dependency on it. This can result in undefined outputs during execution. To ensure that the necessary data is available, add the dependency like so:- runs-on: ubuntu-latest + needs: [ci-docker-tags] + runs-on: ubuntu-latest🧰 Tools
🪛 actionlint (1.7.4)
99-99: property "ci-docker-tags" is not defined in object type {}
(expression)
107-107: property "ci-docker-tags" is not defined in object type {}
(expression)
114-114: property "ci-docker-tags" is not defined in object type {}
(expression)
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
9-23: CI Docker Tags Job: Output Extraction
Theci-docker-tagsjob correctly downloads thedocker_tagsartifact and extracts the Docker tag values foractivitypubandmigrations. Consider adding error handling or a check to ensure thatdocker_tags.txtexists and contains the expected content; this would help avoid unexpected failures in subsequent deployment jobs if the artifact is missing or malformed.
🛑 Comments failed to post (1)
.github/workflows/deploy.yml (1)
48-84:
⚠️ Potential issueDeploy-Staging Job: Missing Dependency Declaration
This job references outputs from theci-docker-tagsjob using expressions such as${{ needs.ci-docker-tags.outputs.migrations_docker_tags }}and${{ needs.ci-docker-tags.outputs.activitypub_docker_tags }}. However, there is no dependency declared using theneeds:keyword, which means these outputs may not be available and could lead to runtime errors. To fix this, add the dependency to ensure that theci-docker-tagsjob completes before this job starts. For example:- name: (staging) Deploy + needs: [ci-docker-tags] + name: (staging) Deploy📝 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.deploy-staging: if: github.ref == 'refs/heads/main' needs: [ci-docker-tags] name: (staging) Deploy environment: build runs-on: ubuntu-latest strategy: matrix: region: [europe-west4, europe-west3] steps: - name: "Auth with Google Cloud" uses: 'google-github-actions/auth@v2' with: credentials_json: ${{ secrets.SERVICE_ACCOUNT_KEY }} - name: "Deploy Migrations to Cloud Run" if: ${{ matrix.region == 'europe-west4' }} uses: 'google-github-actions/deploy-cloudrun@v2' with: image: europe-docker.pkg.dev/ghost-activitypub/activitypub/migrations:${{ needs.ci-docker-tags.outputs.migrations_docker_tags }} region: ${{ matrix.region }} job: stg-${{ matrix.region }}-activitypub-migrations flags: '--wait --execute-now --set-cloudsql-instances=ghost-activitypub:${{ matrix.region }}:stg-${{ matrix.region }}-activitypub-primary' - name: "Deploy ActivityPub Queue to Cloud Run" uses: 'google-github-actions/deploy-cloudrun@v2' with: image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.ci-docker-tags.outputs.activitypub_docker_tags }} region: ${{ matrix.region }} service: stg-${{ matrix.region }}-activitypub-queue - name: "Deploy ActivityPub API to Cloud Run" uses: 'google-github-actions/deploy-cloudrun@v2' with: image: europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:${{ needs.ci-docker-tags.outputs.activitypub_docker_tags }} region: ${{ matrix.region }} service: stg-${{ matrix.region }}-activitypub-api🧰 Tools
🪛 actionlint (1.7.4)
66-66: property "ci-docker-tags" is not defined in object type {}
(expression)
74-74: property "ci-docker-tags" is not defined in object type {}
(expression)
81-81: property "ci-docker-tags" is not defined in object type {}
(expression)
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
.github/workflows/deploy.yml (1)
85-117:⚠️ Potential issueDeploy-Old Job – Ensure Consistent Image Tag References
Similarly, thedeploy-oldjob references outputs from theci-docker-tagsjob (e.g.,${{ needs.ci-docker-tags.outputs.migrations_docker_tags }}). This setup is dependent on the proper declaration of job outputs in theci-docker-tagsjob. Once the outputs are defined at the job level, these references should resolve correctly.🧰 Tools
🪛 actionlint (1.7.4)
99-99: property "ci-docker-tags" is not defined in object type {}
(expression)
107-107: property "ci-docker-tags" is not defined in object type {}
(expression)
114-114: property "ci-docker-tags" is not defined in object type {}
(expression)
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
99-101: Commented Out Test Step – Verify Intentional Disablement
The "Run Tests" step has been commented out. If this is an intentional decision (perhaps because tests are handled elsewhere or temporarily disabled during staging deployments), please add an inline comment explaining the rationale to avoid confusion in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(3 hunks).github/workflows/deploy.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/deploy.yml
66-66: property "ci-docker-tags" is not defined in object type {}
(expression)
74-74: property "ci-docker-tags" is not defined in object type {}
(expression)
81-81: property "ci-docker-tags" is not defined in object type {}
(expression)
99-99: property "ci-docker-tags" is not defined in object type {}
(expression)
107-107: property "ci-docker-tags" is not defined in object type {}
(expression)
114-114: property "ci-docker-tags" is not defined in object type {}
(expression)
🔇 Additional comments (3)
.github/workflows/build.yml (2)
6-11: Enhanced Pull Request Trigger Events
The addition of the extra pull_request event types (opened, synchronize, reopened, labeled, unlabeled) broadens the scope of events triggering this workflow. This ensures that changes affecting PR metadata are caught early.
130-139: Output and Upload Docker Tags Steps Added
The new steps to output Docker tags to a file and subsequently upload them as an artifact correctly support the new CD workflow. Ensure that the file path and artifact name remain consistent with what the deployment workflow expects..github/workflows/deploy.yml (1)
1-8: Continuous Deployment Workflow Trigger Configuration
The CD workflow is now configured to trigger on the completion of the CI workflow viaworkflow_run. This setup is well suited to ensure that deployment steps run only after CI completes successfully.
rmgpinto
force-pushed
the
ephemeral-staging-envs
branch
from
623a891 to
05f4435
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
149-155: Update Checkout Action Version for activitypub-infra Repo
This step is usingactions/checkout@v3, which is flagged as outdated. Upgrading toactions/checkout@v4is recommended for improved reliability.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
151-151: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
157-163: Update Checkout Action Version for Terraform Repo
Similarly, the terraform repo checkout usesactions/checkout@v3. Please update this toactions/checkout@v4.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
159-159: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (2)
.github/workflows/build.yml (2)
134-147: Quote Shell Variable for Robustness
In the label-checking step, the condition usesif [ $LABEL_NAMES != "" ]; thenFor safer shell scripting (avoiding issues if
LABEL_NAMESis empty or contains spaces), please quote the variable as follows:if [ "$LABEL_NAMES" != "" ]; then
177-183: Review Module URL Replacement Command
The sed commands update module URLs correctly. However, the replacement pattern:sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tfmay not substitute the PR number as expected due to the usage of single quotes. Confirm that GitHub Actions processes the expression as intended or consider switching to double quotes to guarantee interpolation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
151-151: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
159-159: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
199-199: property "build-test-push" is not defined in object type {}
(expression)
🔇 Additional comments (8)
.github/workflows/build.yml (8)
3-6: PR Trigger Events Configuration
The pull request trigger now explicitly includes types[opened, synchronize, reopened, labeled], which is appropriate for ephemeral staging environments.
127-132: Deploy-pr Job Configuration
The newdeploy-prjob is properly conditioned on pull request events and configured for the staging environment. The job’s overall structure aligns well with the ephemeral staging purpose.
165-169: Verify Terraform Version File Presence
The command correctly reads the Terraform version from
activitypub-infra/infrastructure/activitypub-staging-environments/.terraform-version
Ensure that this file exists in the repository as expected.
171-176: Terraform Setup Configuration is Correct
The terraform setup step properly uses the output from the previous step. No modifications are needed here.
185-190: Google Cloud Authentication Step is Correct
The authentication step with Google Cloud is configured properly using the provided service account key.
191-195: Terraform Init Step Configuration
The Terraform initialization command in this step is clear and effective.
208-212: Load Balancer Update Step Looks Good
The commands to export and display the current URL maps are straightforward and appear functional.
40-48:Details
❓ Verification inconclusive
Note on Commented-out
build-test-pushJob
Thebuild-test-pushjob block is currently commented out. However, its outputs are referenced later in the Terraform apply step. This discrepancy can lead to undefined output errors during deployment.Please consider re-enabling this job or updating the dependent steps to source these outputs from a valid alternative.
Below is a shell script to search for references to the commented-out job outputs and the job itself. This will help verify if outputs from the
build-test-pushjob (i.e.migrations_docker_versionandactivitypub_docker_version) are indeed used later—especially in parts of the Terraform workflow. Once these commands run, we can better decide if re-enabling this job or updating downstream steps is required.
🏁 Script executed:
#!/bin/bash echo "Searching for references of 'migrations_docker_version' in the repository:" rg "migrations_docker_version" -n . echo "Searching for references of 'activitypub_docker_version' in the repository:" rg "activitypub_docker_version" -n . echo "Searching for references to the commented-out job 'build-test-push':" rg "build-test-push" -n . echo "Searching for references to 'terraform apply' (to check dependent steps):" rg "terraform apply" -n .Length of output: 774
Action Required: Verify Downstream Dependencies for Commented-Out Job Outputs
The commented-out
build-test-pushjob (lines 40-48 in.github/workflows/build.yml) defines outputs (migrations_docker_versionandactivitypub_docker_version) that appear to be expected by subsequent steps (e.g., during Terraform apply operations). However, our initial automated search did not conclusively reveal references to these outputs within the repository.Before proceeding, please manually verify whether downstream steps (in Terraform configurations or other workflow files) depend on these outputs. If they do, consider either re-enabling the
build-test-pushjob or updating the dependent steps to source these values from a valid alternative.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
168-174: 🛠️ Refactor suggestionUpdate Checkout Action Version for Terraform Repo
Similarly, the "Checkout terraform repo" step is still usingactions/checkout@v3. Upgrading toactions/checkout@v4will align with current best practices and static analysis recommendations.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
160-166: 🛠️ Refactor suggestionUpdate Checkout Action Version for activitypub-infra Repository
The "Checkout activitypub-infra repo" step usesactions/checkout@v3, which is outdated. It is recommended to update toactions/checkout@v4for better performance and compatibility.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
150-158: Enhance Shell Script Robustness by Quoting Variables
Within the label check, the conditionif [ $LABEL_NAMES != "" ]; thenshould quote the variable to safeguard against cases when it is empty. This will prevent potential shell errors.
- if [ $LABEL_NAMES != "" ]; then + if [ "$LABEL_NAMES" != "" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
212-212: property "build-test-push" is not defined in object type {}
(expression)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
160-163: 🛠️ Refactor suggestionUpgrade Checkout Action for activitypub-infra Repository
The checkout step is using
actions/checkout@v3, which has been flagged as outdated. Upgrading toactions/checkout@v4is recommended for reliability and compatibility.- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
168-171: 🛠️ Refactor suggestionUpgrade Checkout Action for Terraform Repository
Similarly, the checkout step for the Terraform repository is still using
actions/checkout@v3. Please update it toactions/checkout@v4:- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
145-158: Quote Environment Variable in Label CheckIn the step that checks for matching labels, the if-condition currently reads:
if [ $LABEL_NAMES != "" ]; thenTo avoid potential issues when
$LABEL_NAMESis empty or contains spaces, it’s best to quote the variable:- if [ $LABEL_NAMES != "" ]; then + if [ "$LABEL_NAMES" != "" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
211-211: property "build-test-push" is not defined in object type {}
(expression)
🔇 Additional comments (7)
.github/workflows/build.yml (7)
138-142: Job Definition fordeploy-prLooks GoodThe new
deploy-prjob is correctly defined with the appropriate conditions for pull request events. However, please ensure that the intended behavior regarding dependency outputs is consistent throughout the workflow.
176-181: Terraform Version Retrieval Step ValidationThe "Get terraform version" step correctly retrieves the version from the specified file using:
cat activitypub-infra/infrastructure/activitypub-staging-environments/.terraform-versionThis step appears to be correct and consistent with expectations.
182-187: Terraform Setup Step is CorrectThe step using
hashicorp/setup-terraform@v3with the interpolated terraform version is properly configured.
188-194: Module URL Replacement Implementation is SoundChanging GitHub URL references with
sedto point to local directories and adjusting backend prefixes is implemented correctly. No issues noted.
196-202: GCP Authentication Step is CorrectThe authentication with GCP via
google-github-actions/auth@v2is properly set up with the required inputs.
203-208: Terraform Init Step is Implemented CorrectlyThe steps for initializing Terraform in the designated directory are clear and correct.
220-225: Load Balancer Update Step is ClearThe steps to update the load balancer using
gcloud compute url-maps exportare implemented correctly.
closes https://linear.app/ghost/issue/AP-976 - Create ephemeral staging environment on PR labels *.ghost.is
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
160-167: 🛠️ Refactor suggestionUpdate Checkout Action Version for activitypub-infra Repo
The step for checking out theactivitypub-infrarepo usesactions/checkout@v3, which is flagged as outdated. Upgrading toactions/checkout@v4will improve reliability. Consider applying the following diff:- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
168-175: 🛠️ Refactor suggestionUpdate Checkout Action Version for Terraform Repo
Similarly, the checkout step for the terraform repo also usesactions/checkout@v3. It is advisable to update this to version v4 for consistency and to meet best practices. For example:- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (4)
.github/workflows/build.yml (4)
145-158: Quote Shell Variables in the Label Check Step
In the shell conditional on line 152, the variable expansion ofLABEL_NAMESis unquoted. To prevent potential issues (especially with empty strings or values containing spaces), it’s recommended to use quotes, e.g.,- if [ $LABEL_NAMES != "" ]; then + if [ "$LABEL_NAMES" != "" ]; then
188-195: Verify Variable Substitution in Sed Command
The step that changes module URLs uses a sed command on line 194:sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tfEnsure that the GitHub Actions YAML engine correctly substitutes the PR number in this context. In some cases, using double quotes around the sed expression might be necessary for proper substitution. Please verify that the intended PR number appears in the resulting file.
209-219: Terraform Apply Step Uses Static Docker Tags
Theterraform applystep exports docker image environment variables with static:edgetags. Please confirm that using these static tags is the intended design now that the outputs from the previously defined (but commented out)build-test-pushjob are no longer available. If dynamic versioning is required, consider parameterizing these values in the future.
229-345: Clean Up Commented-Out Jobs if No Longer Needed
There remains a significant amount of commented-out code for jobs such aslint,check-yarn-lock,build-test-push, and the deployment jobs. If these are deprecated in favor of the new ephemeral staging workflow, consider removing them to reduce clutter and avoid any confusion in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (6)
.github/workflows/build.yml (6)
138-142: Ephemeral Staging Deploy Job Definition Looks Good
The newdeploy-prjob is well defined with a clear condition for pull request events. The environment and runner configuration match the intended usage for ephemeral staging environments.
176-180: Terraform Version Retrieval Step Looks Correct
The step correctly references the fileactivitypub-infra/infrastructure/activitypub-staging-environments/.terraform-version. No issues were identified here.
182-187: Terraform Setup Step is Properly Configured
The use of the output from theterraform-versionstep in thesetup-terraformaction is correct and follows the GitHub Actions expression syntax.
196-202: GCP Authentication Step is Configured Correctly
The authentication step correctly sets up access using the specified service account. No changes are needed here.
203-208: Terraform Initialization is Straightforward
Theterraform initstep properly changes directory and initializes Terraform. This step is clear and efficient.
223-228: Load Balancer Update Step Looks Good
The step to export and display the current URL maps is clear and straightforward. No issues detected.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build.yml (2)
160-167: 🛠️ Refactor suggestionUpgrade Checkout Action Version for ActivityPub Infra Repository
The step "Checkout activitypub-infra repo" is currently using
actions/checkout@v3, which is flagged by static analysis as outdated. Upgrading toactions/checkout@v4will improve reliability and performance. For example:- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
168-175: 🛠️ Refactor suggestionUpgrade Checkout Action Version for Terraform Repository
Similarly, update the checkout step for the terraform repository from
actions/checkout@v3toactions/checkout@v4:- uses: actions/checkout@v3 + uses: actions/checkout@v4🧰 Tools
🪛 actionlint (1.7.4)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🧹 Nitpick comments (2)
.github/workflows/build.yml (2)
138-144: Verify Newdeploy-prJob Configuration and Dependency ManagementThe new
deploy-prjob is well defined with a clear conditional trigger for pull request events and an appropriate environment. However, note that theneeds: [build-test-push]dependency is commented out. Please double-check that this omission is intentional and that no outputs from thebuild-test-pushjob are required downstream in this job.
145-158: Improve Label Check Safety in Shell ScriptIn the "Check if any label matches *.ghost.is" step, the condition:
if [ $LABEL_NAMES != "" ]; thencould lead to word-splitting issues when
LABEL_NAMESis empty or contains spaces. Consider replacing it with a more robust test:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis adjustment ensures proper behavior regardless of the variable content.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
209-218: Confirm Hardcoded 'edge' Image Tags in Terraform Apply StepIn the "Terraform apply" step, environment variables for Docker images are exported with a hardcoded
edgetag:export TF_VAR_migrations_image=europe-docker.pkg.dev/ghost-activitypub/activitypub/migrations:edge export TF_VAR_api_image=europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:edge export TF_VAR_queue_image=europe-docker.pkg.dev/ghost-activitypub/activitypub/activitypub:edgePlease verify that using the
edgetag is intentional for ephemeral staging environments. If a dynamic or versioned tag is desired, consider modifying these assignments accordingly.
| - name: "Change github.com url in modules to local directories and add backend prefix" | ||
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | ||
| run: | | ||
| cd activitypub-infra/infrastructure/activitypub-staging-environments | ||
| sed -i 's/github\.com\/TryGhost/\.\.\/\.\.\/\.\./gI' main.tf | ||
| sed -i 's/\?ref=main//g' main.tf | ||
| sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tf | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Correct Sed Substitution for GitHub Expression
In the "Change github.com url in modules…" step, the sed command on line 193 uses single quotes:
sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tfSince single quotes inhibit shell interpolation, the GitHub Actions expression may not be expanded as intended. Consider using double quotes:
- sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tf
+ sed -i "s/REPLACE_ME/${{ github.event.pull_request.number }}/g" terraform.tfThis change should ensure that the PR number is correctly substituted in terraform.tf.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
193-193: 🛠️ Refactor suggestionUse double quotes for sed substitution to interpolate PR number
The sed command uses single quotes, which prevent shell/GitHub Actions expression interpolation. Use double quotes to ensure
${{ github.event.pull_request.number }}is correctly substituted:- sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tf + sed -i "s/REPLACE_ME/${{ github.event.pull_request.number }}/g" terraform.tf
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
151-151: Quote shell variable in conditional for robustnessThe conditional
if [ $LABEL_NAMES != "" ]; thenmay fail if$LABEL_NAMESis unset or contains spaces. For safer shell scripting, quote the variable:- if [ $LABEL_NAMES != "" ]; then + if [ "$LABEL_NAMES" != "" ]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
161-161: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
169-169: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
138-227: Ephemeral staging deployment logic is sound and aligns with PR objectivesThe new
deploy-prjob correctly implements ephemeral staging environments, conditionally deploying based on PR labels and using infrastructure as code best practices. Previous issues with undefined outputs and variable naming have been addressed. The use of hardcoded Docker image tags is acceptable for ephemeral environments.🧰 Tools
🪛 actionlint (1.7.4)
161-161: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
169-169: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
138-345: Ephemeral staging workflow logic looks solidThe new
deploy-prjob and its conditional logic for ephemeral staging environments are well-structured and align with the PR objectives. Previous issues with output variables, path typos, and undefined dependencies have been addressed. The workflow now provides a clear, maintainable, and flexible approach for PR-based ephemeral deployments.🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
194-194: 🛠️ Refactor suggestionFix shell interpolation for PR number substitution in sed command.
The sed command for replacing
REPLACE_MEwith the PR number uses single quotes, which prevents shell interpolation of${{ github.event.pull_request.number }}. Use double quotes to ensure the PR number is correctly substituted.- sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tf + sed -i "s/REPLACE_ME/${{ github.event.pull_request.number }}/g" terraform.tf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
138-228: Ephemeral staging deployment logic is well-structured and aligns with PR objectives.The new
deploy-prjob correctly implements conditional ephemeral staging deployments based on PR labels, checks out required repos, sets up Terraform, and applies infrastructure changes. This approach improves flexibility and supports modern development workflows.🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
194-194: 🛠️ Refactor suggestionFix PR number substitution in sed command
The sed command for substituting the PR number uses single quotes, which prevents GitHub Actions expressions from being interpolated. Use double quotes to ensure the PR number is correctly substituted in
terraform.tf.- sed -i 's/REPLACE_ME/${{ github.event.pull_request.number }}/g' terraform.tf + sed -i "s/REPLACE_ME/${{ github.event.pull_request.number }}/g" terraform.tf
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/build.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🔇 Additional comments (1)
.github/workflows/build.yml (1)
138-239: Ephemeral staging workflow logic looks solidThe new
deploy-prjob is well-structured, conditionally triggered, and implements ephemeral staging environments as described in the PR objectives. The use of hardcodededgetags for Docker images is reasonable for temporary environments. No critical issues found in the overall workflow logic.🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
| uses: docker/metadata-action@v5 | ||
| - name: "Checkout activitypub-infra repo" | ||
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Upgrade actions/checkout to v4 for compatibility and security
Both the "Checkout activitypub-infra repo" and "Checkout terraform repo" steps use actions/checkout@v3, which is outdated and flagged by static analysis. Upgrade to actions/checkout@v4 for continued compatibility and improved security.
- uses: actions/checkout@v3
+ uses: actions/checkout@v4[static_analysis]
Also applies to: 170-170
🧰 Tools
🪛 actionlint (1.7.4)
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/cd-staging-environments.yml (1)
1-112: Well-structured workflow for ephemeral staging environments.The workflow is thoughtfully designed, conditionally deploying ephemeral environments based on PR labels, and integrates well with Terraform and GCP. The use of label gating, dynamic Terraform versioning, and PR-specific resource naming is robust.
Consider documenting the teardown process for these ephemeral environments, and optionally add error handling or notifications for failed applies to improve maintainability and developer experience.
🧰 Tools
🪛 actionlint (1.7.4)
37-37: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(2 hunks).github/workflows/cd-staging-environments.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cd-staging-environments.yml
37-37: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
45-45: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
|
|
||
| - name: "Checkout activitypub-infra repo" | ||
| if: ${{ steps.check-labels.outputs.is_ephemeral_staging == 'true' }} | ||
| uses: actions/checkout@v3 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update actions/checkout to v4 for compatibility.
Static analysis indicates that actions/checkout@v3 is too old for the current GitHub Actions runner. Please update both usages to actions/checkout@v4 to ensure compatibility and receive the latest security and performance improvements.
-uses: actions/checkout@v3
+uses: actions/checkout@v4Also applies to: 45-45
🧰 Tools
🪛 actionlint (1.7.4)
37-37: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/cicd.yml (2)
188-195: Terraform config mutation: ensure idempotency and error handlingThe step that modifies
main.tfandterraform.tfusessedto replace URLs and a placeholder. This is potentially brittle:
- If the step is re-run, repeated
sedreplacements could corrupt the files.- No error handling is present if the files or patterns are missing.
Consider:
- Backing up the files before mutation.
- Using
sed -i.bakand restoring from backup on failure.- Adding
set -eat the top of the script to fail fast on errors.Optionally, check that the replacements actually occur and fail if not.
138-371: Commented-out jobs: keep configuration up to dateMost jobs (lint, build, deploy-staging, deploy-production) are commented out. While this is fine for incremental rollout, ensure that:
- The commented code is kept in sync with the active workflow.
- Any secrets or environment variables referenced are still valid.
- The team is aware of which jobs are intentionally disabled.
Consider adding a comment at the top of the file explaining the rationale for the commented-out jobs and the plan for re-enabling them.
🧰 Tools
🪛 actionlint (1.7.4)
138-138: "steps" section is missing in job "deploy-pr"
(syntax-check)
138-138: "runs-on" section is missing in job "deploy-pr"
(syntax-check)
138-138: "deploy-pr" job should not be empty. please remove this section if it's unnecessary
(syntax-check)
139-139: "steps" section is missing in job "if"
(syntax-check)
139-139: "runs-on" section is missing in job "if"
(syntax-check)
139-139: "if" job is scalar node but mapping node is expected
(syntax-check)
140-140: "steps" section is missing in job "name"
(syntax-check)
140-140: "runs-on" section is missing in job "name"
(syntax-check)
140-140: "name" job is scalar node but mapping node is expected
(syntax-check)
141-141: "steps" section is missing in job "runs-on"
(syntax-check)
141-141: "runs-on" section is missing in job "runs-on"
(syntax-check)
141-141: "runs-on" job is scalar node but mapping node is expected
(syntax-check)
143-143: "steps" section is missing in job "environment"
(syntax-check)
143-143: "runs-on" section is missing in job "environment"
(syntax-check)
143-143: "environment" job is scalar node but mapping node is expected
(syntax-check)
144-144: "steps" section is missing in job "steps"
(syntax-check)
144-144: "runs-on" section is missing in job "steps"
(syntax-check)
145-145: "steps" job is sequence node but mapping node is expected
(syntax-check)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/build.yml(0 hunks).github/workflows/cicd.yml(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/build.yml
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cicd.yml
138-138: "steps" section is missing in job "deploy-pr"
(syntax-check)
138-138: "runs-on" section is missing in job "deploy-pr"
(syntax-check)
138-138: "deploy-pr" job should not be empty. please remove this section if it's unnecessary
(syntax-check)
139-139: "steps" section is missing in job "if"
(syntax-check)
139-139: "runs-on" section is missing in job "if"
(syntax-check)
139-139: "if" job is scalar node but mapping node is expected
(syntax-check)
140-140: "steps" section is missing in job "name"
(syntax-check)
140-140: "runs-on" section is missing in job "name"
(syntax-check)
140-140: "name" job is scalar node but mapping node is expected
(syntax-check)
141-141: "steps" section is missing in job "runs-on"
(syntax-check)
141-141: "runs-on" section is missing in job "runs-on"
(syntax-check)
141-141: "runs-on" job is scalar node but mapping node is expected
(syntax-check)
143-143: "steps" section is missing in job "environment"
(syntax-check)
143-143: "runs-on" section is missing in job "environment"
(syntax-check)
143-143: "environment" job is scalar node but mapping node is expected
(syntax-check)
144-144: "steps" section is missing in job "steps"
(syntax-check)
144-144: "runs-on" section is missing in job "steps"
(syntax-check)
145-145: "steps" job is sequence node but mapping node is expected
(syntax-check)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
150-158: 🛠️ Refactor suggestionLabel check logic: use robust string check for empty variables
The current check
[ $LABEL_NAMES != "" ]is fragile and may fail ifLABEL_NAMESis unset or empty, especially in strict shells. For robustness, quote the variable and use-n:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and is the recommended shell idiom.
233-254: 🛠️ Refactor suggestionLoad balancer update: ensure atomicity, error handling, and cleanup
This step exports the URL map, modifies it with
yq, but does not re-import the config to GCP, lacks error handling, and leaves temp files behind. For a robust and atomic update:
- Add
set -eat the top for error handling.- After modifying, re-import the config with
gcloud compute url-maps import ...to apply changes.- Clean up temporary files (
config.yml.tmp).- If this is only a preview, clarify intent in a comment.
Example improvement:
set -e gcloud compute url-maps export stg-activitypub --global --project ghost-activitypub > config.yml # ... yq logic ... cp config.yml.tmp config.yml gcloud compute url-maps import stg-activitypub --global --project ghost-activitypub --source=config.yml rm -f config.yml.tmp
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cicd.yml
162-162: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
170-170: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/cicd.yml (2)
149-162: 🛠️ Refactor suggestionLabel check logic: quote variables for safety
The label check step uses
[ $LABEL_NAMES != "" ], which can fail if$LABEL_NAMESis unset or contains spaces. For robustness, quote the variable and use-n:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents errors in strict shells and with empty or multi-word label lists.
237-295: 🛠️ Refactor suggestionImprove error handling and cleanup in Load Balancer update step
The load balancer update step performs several file operations and external commands (
gcloud,yq,jq,mktemp). For reliability and maintainability:
- Add
set -euo pipefailat the top to fail on errors, unset variables, or pipeline failures.- Clean up temporary files created by
mktempafter use.- Check for errors after critical commands (e.g.,
gcloud,yq).- Consider using
trapto ensure cleanup even if the script fails.Example improvements:
+ set -euo pipefail ... - HEADER_MATCHES_YAML=$(mktemp) + HEADER_MATCHES_YAML=$(mktemp) ... - ROUTE_RULE=$(mktemp) + ROUTE_RULE=$(mktemp) ... # After use: + rm -f "$HEADER_MATCHES_YAML" "$ROUTE_RULE"This will make the deployment step more robust and prevent resource leaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cicd.yml
166-166: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
174-174: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
149-162: 🛠️ Refactor suggestionLabel check logic: ensure robust handling of empty or unset variables
The label check step uses
jqto extract label names and checks[ $LABEL_NAMES != "" ]. This can fail if$LABEL_NAMESis unset or empty, especially in strict shells. For robustness, quote the variable and use-n:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and ensures correct behavior when no matching labels are present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cicd.yml
166-166: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
174-174: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
155-168: Label check logic is robust and safe.The label check step correctly uses
jqto extract PR labels and checks for matches to*.ghost.is. The use ofif [ $LABEL_NAMES != "" ];was previously flagged as unsafe if$LABEL_NAMESis unset, but this has been addressed in a past review. Ensure the variable is always quoted ([ -n "$LABEL_NAMES" ]) to avoid unbound variable errors in strict shells.
🧹 Nitpick comments (2)
.github/workflows/cicd.yml (2)
243-273: Load balancer update: error handling, atomicity, and cleanupThe load balancer update step exports, modifies, and prepares to re-import the URL map. This logic is robust and follows best practices, as previously discussed:
- Temporary files are used for atomic updates.
- Unnecessary fields are deleted.
- Host rules and path matchers are ensured.
- Route rules are updated based on PR labels.
Suggestions:
- Add
set -euo pipefailat the top of the script for strict error handling.- Explicitly clean up temporary files (e.g.,
config.yml.tmp) at the end of the step.- Add a comment clarifying that the import step is intentionally commented out for preview/testing, and should be enabled for production.
1-390: General workflow structure and future-proofingThe overall workflow is well-structured for future expansion. Most jobs are commented out, focusing current execution on ephemeral staging deployments. When re-enabling jobs (lint, build, deploy-staging, deploy-production), ensure:
- All action versions are up to date (e.g.,
actions/checkout@v4).- Any new steps follow the same error handling and atomicity standards as the active steps.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
162-162: 🛠️ Refactor suggestionLabel check logic: use robust string test to avoid shell errors
The label check uses
[ $LABEL_NAMES != "" ], which can fail if$LABEL_NAMESis unset or empty. Use[ -n "$LABEL_NAMES" ]for a safe, POSIX-compliant check:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and works even if no labels are present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/cicd.yml (2)
277-277: Config not re-imported: clarify intent or uncomment import stepThe script modifies the load balancer config but does not re-import it into GCP (the import step is commented out). If the intent is to apply changes, uncomment the import command:
# gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quietIf this is only a preview, clarify with a comment.
26-26: actions/checkout@v4: Good use of latest versionThe workflow uses
actions/checkout@v4, which is the recommended and secure version.Also applies to: 38-38, 59-59
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
.github/workflows/cicd.yml (3)
274-274:⚠️ Potential issueYAML update: fix typo in yq command for routeAction
The
yqcommand at this line has a typo:routeAction.weightedBackendServices: [...]is invalid YAML. It should be:- "routeAction.weightedBackendServices: [ { \"backendService\": \"...\", \"weight\": 100 } ]" + "routeAction: { weightedBackendServices: [ { \"backendService\": \"...\", \"weight\": 100 } ] }"Update the
yqcommand to use the correct YAML structure forrouteAction.
155-168: 🛠️ Refactor suggestionLabel check logic: quote variables and use
-nfor robustnessThe label check step uses
[ $LABEL_NAMES != "" ], which can fail if$LABEL_NAMESis unset or empty, especially withset -uor stricter shells. For safety and clarity, quote the variable and use-n:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and is more idiomatic in shell scripts.
243-279: 🛠️ Refactor suggestionLoad balancer update: fix YAML structure, add error handling, and clean up temp files
- The
yqcommand at line 274 has a typo in the YAML structure forrouteAction. It should be:
- "routeAction": {"weightedBackendServices: [ ... ] }
- "routeAction": { "weightedBackendServices": [ ... ] }
This ensures the YAML is valid and matches the expected GCP schema.
Add
set -euo pipefailat the top of the script to fail fast on errors, avoid unset variables, and catch pipeline failures.Use
trapto clean up temporary files (e.g.,config.yml.tmp) even if the script fails.Consider adding comments to clarify each block's intent for maintainability.
Suggested changes:
+ set -euo pipefail + trap 'rm -f config.yml.tmp' EXIT ... - yq '.pathMatchers[0].routeRules += [{"priority": '"$NEXT_PRIORITY"', "matchRules": [{"prefixMatch": "/", "headerMatches": '$HEADER_MATCHES'}], "routeAction": {"weightedBackendServices: [ { "backendService": "'"$PR_SERVICE"'", "weight": 100 } ] } }]' config.yml > config.yml.tmp + yq '.pathMatchers[0].routeRules += [{"priority": '"$NEXT_PRIORITY"', "matchRules": [{"prefixMatch": "/", "headerMatches": '$HEADER_MATCHES'}], "routeAction": { "weightedBackendServices": [ { "backendService": "'"$PR_SERVICE"'", "weight": 100 } ] } }]' config.yml > config.yml.tmpThese changes will make the deployment more robust and easier to debug.
🧹 Nitpick comments (1)
.github/workflows/cicd.yml (1)
172-174: Updateactions/checkoutto v4 for compatibility and securityThe commented-out steps use
actions/checkout@v3. Update these toactions/checkout@v4to ensure compatibility with the latest GitHub Actions runners and benefit from security updates.- uses: actions/checkout@v3 + uses: actions/checkout@v4Also applies to: 180-182
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
162-162: 🛠️ Refactor suggestionQuote shell variables and use
-nfor robust empty checks in shell scripts.The conditional
if [ $LABEL_NAMES != "" ]; thenis unsafe if$LABEL_NAMESis unset or empty, which can cause errors in strict shells. Use[ -n "$LABEL_NAMES" ]instead, and always quote shell variables in conditionals.- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and is a best practice for shell scripting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/cicd.yml (1)
1-392: Overall workflow structure and ephemeral staging logic look good.The workflow is well-structured, aligns with the PR objective of ephemeral staging environments, and uses modern GitHub Actions and GCP authentication patterns. The commented-out jobs provide a clear path for future expansion.
Once the above shell script improvements are made, the workflow will be robust and production-ready.
| # Get current config | ||
| gcloud compute url-maps export stg-activitypub --global --project ${GCP_PROJECT} > config.yml | ||
| # Delete unnecessary fields | ||
| yq 'del(.fingerprint)' config.yml -i | ||
| yq 'del(.creationTimestamp)' config.yml -i | ||
| 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.infra.ghost.is"], "pathMatcher": "staging-environments"}])' config.yml > config.yml.tmp | ||
| yq '.hostRules = ([{"hosts": ["activitypub.infra.ghost.is"], "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 | ||
| yq '.pathMatchers = ([{"name": "staging-environments", "defaultService": "'"$DEFAULT_SERVICE"'", "routeRules": []}])' config.yml > config.yml.tmp | ||
| mv config.yml.tmp 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 | ||
| # Add new route rules for the PR service | ||
| export MAX_PRIORITY=$(yq '[.pathMatchers[] | select(.name == "staging-environments") | .routeRules[]?.priority] | max // 0' config.yml) | ||
| export NEXT_PRIORITY=$((MAX_PRIORITY + 1)) | ||
| export HEADER_MATCHES=$(echo "$LABELS" | jq -c '[.[] | select(.name | test("\\.ghost\\.is$")) | { "headerName": "X-Forwarded-Host", "exactMatch": "\(.name)" }']) | ||
| yq '.pathMatchers[0].routeRules += [{"priority": '"$NEXT_PRIORITY"', "matchRules": [{"prefixMatch": "/", "headerMatches": '$HEADER_MATCHES'}], "routeAction": {"weightedBackendServices": [ { "backendService": "'$PR_SERVICE'", "weight": 100 } ] } }]' config.yml > config.yml.tmp | ||
| mv config.yml.tmp config.yml | ||
| echo "Updating url map with:" | ||
| cat config.yml | ||
| gcloud compute url-maps import stg-activitypub --source=config.yml --global --project ${GCP_PROJECT} --quiet |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling and cleanup to the load balancer update script.
For reliability, add set -euo pipefail at the top of the script to fail fast on errors, and a trap to clean up temporary files even if the script fails. This prevents silent failures and orphaned temp files.
Add these lines at the top of the run block:
set -euo pipefail
trap 'rm -f config.yml.tmp' EXITThis will make the deployment more robust and easier to debug.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
156-170:⚠️ Potential issueHarden label check logic for empty or unset variables
The label check step uses:
if [ $LABEL_NAMES != "" ]; thenThis is fragile and can fail if
LABEL_NAMESis unset or empty, especially withset -uor stricter shells. For robustness, always quote the variable and use-n:- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and is the recommended shell idiom.
Apply this diff to fix:
- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; then
🧹 Nitpick comments (1)
.github/workflows/cicd.yml (1)
277-289: "WIP" step: Consider clarifying intent and making PR number dynamicThe "WIP" step lists Cloud Run services and checks the state of PR 432 via
gh pr view 432. This is hardcoded and will not work for other PRs. If the intent is to check the state of the current PR, use${{ github.event.pull_request.number }}or similar.Also, consider adding comments to clarify the purpose of this step for future maintainers.
Apply this diff to make the PR number dynamic:
- gh pr view 432 --json state | jq -r '.state' + gh pr view $num --json state | jq -r '.state'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/cicd.yml (1)
207-214: GCP authentication step is correct and follows best practicesThe GCP authentication step uses workload identity and a dedicated service account for ephemeral staging environments. This is a secure and recommended approach for GitHub Actions workflows.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/cicd.yml (1)
163-163:⚠️ Potential issueQuote shell variables in conditional checks for robustness
In the label check step, the conditional
if [ $LABEL_NAMES != "" ];should quote the variable to prevent errors if$LABEL_NAMESis unset or contains spaces. Use[ -n "$LABEL_NAMES" ]for a safe, POSIX-compliant check.- if [ $LABEL_NAMES != "" ]; then + if [ -n "$LABEL_NAMES" ]; thenThis prevents unbound variable errors and improves script reliability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/cicd.yml(5 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/cicd.yml
173-173: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
181-181: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build, Test and Push
🔇 Additional comments (3)
.github/workflows/cicd.yml (3)
245-275: Excellent: Load balancer update script is robust and follows best practicesThe load balancer update step now includes proper error handling, atomic file operations, and re-imports the modified config. Temporary files are managed and the logic is clear. This addresses all previously raised concerns.
109-140: Conditional Docker push logic is correct and efficientThe conditional execution of GCP authentication, Artifact Registry login, and Docker image pushes ensures that these steps only run for relevant pull request actions. This optimizes workflow runs and avoids unnecessary operations.
149-243: Ephemeral staging deployment logic is well-structuredThe new
deploy-prjob is well-organized, with clear separation of label checking, repo checkouts, Terraform setup, GCP authentication, and deployment steps. The use of environment variables and conditional execution is appropriate.🧰 Tools
🪛 actionlint (1.7.4)
173-173: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
181-181: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
There was a problem hiding this comment.
Pull Request Overview
This PR adds ephemeral staging environments by updating the CICD workflow triggers, modifying job environments, and introducing a new deploy-pr job to conditionally deploy based on pull request labels.
- Updated pull request event types and conditions in the workflow.
- Changed the build-test-push job’s environment from "staging" to "build".
- Added a deploy-pr job with label checking and load balancer updates for ephemeral staging.
| run: | | ||
| export LABEL_NAMES=$(echo "$LABELS" | jq -r '[.[] | select(.name | test("\\.ghost\\.is$")) | .name] | join(",")') | ||
| echo "Label names: $LABEL_NAMES" | ||
| if [ $LABEL_NAMES != "" ]; then |
There was a problem hiding this comment.
Consider quoting $LABEL_NAMES (e.g. if [ "$LABEL_NAMES" != "" ]; then) to prevent word splitting or potential errors when the variable is empty.
| if [ $LABEL_NAMES != "" ]; then | |
| if [ "$LABEL_NAMES" != "" ]; then |
| 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.infra.ghost.is"], "pathMatcher": "staging-environments"}])' config.yml > config.yml.tmp | ||
| yq '.hostRules = ([{"hosts": ["activitypub.infra.ghost.is"], "pathMatcher": "staging-environments"}])' config.yml > config.yml.tmp |
There was a problem hiding this comment.
The .hostRules field is being set consecutively (lines 258 and 259); if the duplication is not intentional, please remove the redundant command to avoid unexpected overwrites.
| yq '.hostRules = ([{"hosts": ["activitypub.infra.ghost.is"], "pathMatcher": "staging-environments"}])' config.yml > config.yml.tmp |
| build-test-push: | ||
| name: Build, Test and Push | ||
| environment: staging | ||
| environment: build |
There was a problem hiding this comment.
[nitpick] The build-test-push job’s environment was changed from 'staging' to 'build'. Please confirm that this alteration is intentional and does not inadvertently affect deployment behavior.
| environment: build | |
| environment: staging |
closes https://linear.app/ghost/issue/AP-976 - Create ephemeral staging environment on PR labels *.ghost.is
closes https://linear.app/ghost/issue/AP-976