Conversation
akshat5302
requested review from
mguptahub,
pablohashescobar and
sriramveeraghanta
WalkthroughThe GitHub Actions workflow was updated to detect changed files grouped by project areas and switched the proxy Docker build context from Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant ChangedFilesAction
participant DockerBuild
participant DockerCompose
Workflow->>ChangedFilesAction: Detect changed files by project area
ChangedFilesAction-->>Workflow: Provide changed file groups
Workflow->>DockerBuild: Build Docker image with updated context and scopes
DockerBuild-->>Workflow: Build complete
Workflow->>DockerCompose: Deploy with updated proxy config and volumes
DockerCompose-->>Workflow: Services started with SSL and multi-port setup
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
Outside diff range, codebase verification and nitpick comments (2)
deploy/selfhost/docker-compose.yml (2)
205-205: Add a new line character at the end of the file.The file is missing a new line character at the end.
Apply this diff to fix the issue:
+Tools
yamllint
[error] 205-205: no new line character at the end of file
(new-line-at-end-of-file)
[error] 205-205: trailing spaces
(trailing-spaces)
205-205: Remove trailing spaces at the end of the file.The file has trailing spaces at the end.
Apply this diff to fix the issue:
-Tools
yamllint
[error] 205-205: no new line character at the end of file
(new-line-at-end-of-file)
[error] 205-205: trailing spaces
(trailing-spaces)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- .github/workflows/build-branch.yml (1 hunks)
- caddy/Caddyfile.template (1 hunks)
- caddy/Dockerfile (1 hunks)
- caddy/caddy.sh (1 hunks)
- deploy/selfhost/docker-compose.yml (6 hunks)
- deploy/selfhost/variables.env (2 hunks)
Additional context used
Shellcheck
caddy/caddy.sh
[warning] 3-3: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 3-3: Quote this to prevent word splitting.
(SC2046)
GitHub Check: Codacy Static Code Analysis
caddy/Dockerfile
[warning] 1-1: caddy/Dockerfile#L1
Using latest is prone to errors if the image will ever update. Pin the version explicitly to a release tag
yamllint
deploy/selfhost/docker-compose.yml
[error] 205-205: no new line character at the end of file
(new-line-at-end-of-file)
[error] 205-205: trailing spaces
(trailing-spaces)
Additional comments not posted (25)
caddy/caddy.sh (1)
4-4: LGTM!The line is correctly implemented.
The code changes are approved.
caddy/Dockerfile (1)
3-9: LGTM!The lines are correctly implemented.
The code changes are approved.
caddy/Caddyfile.template (3)
1-17: LGTM!The section is correctly implemented.
The code changes are approved.
19-28: LGTM!The section is correctly implemented.
The code changes are approved.
30-32: LGTM!The section is correctly implemented.
The code changes are approved.
deploy/selfhost/variables.env (4)
3-3: LGTM!The
SSLvariable is correctly added with a default value offalse.The code changes are approved.
33-33: LGTM!The
CERT_EMAILvariable is correctly added with a default value ofakshatjain9782@gmail.com.The code changes are approved.
34-34: LGTM!The
CERT_ACME_CAvariable is correctly added with a default value ofhttps://acme-v02.api.letsencrypt.org/directory.The code changes are approved.
35-35: LGTM!The
TRUSTED_PROXIESvariable is correctly added with a default value of0.0.0.0/0.The code changes are approved.
deploy/selfhost/docker-compose.yml (15)
1-9: LGTM!The
x-db-envanchor is correctly added to encapsulate PostgreSQL environment variables, improving modularity and reducing redundancy.The code changes are approved.
10-14: LGTM!The
x-redis-envanchor is correctly added to encapsulate Redis environment variables, improving modularity and reducing redundancy.The code changes are approved.
15-25: LGTM!The
x-data-store-envanchor is correctly added to encapsulate MinIO environment variables, improving modularity and reducing redundancy.The code changes are approved.
26-34: LGTM!The
x-proxy-envanchor is correctly added to encapsulate proxy environment variables, improving modularity and reducing redundancy.The code changes are approved.
97-98: LGTM!The
environmentin serviceapiis correctly updated to include<<: [ *app-env, *db-env, *redis-env, *data-store-env, *proxy-env ], improving maintainability and clarity.The code changes are approved.
111-112: LGTM!The
environmentin serviceworkeris correctly updated to include<<: [ *app-env, *db-env, *redis-env, *data-store-env, *proxy-env ], improving maintainability and clarity.The code changes are approved.
126-127: LGTM!The
environmentin servicebeat-workeris correctly updated to include<<: [ *app-env, *db-env, *redis-env, *data-store-env, *proxy-env ], improving maintainability and clarity.The code changes are approved.
141-142: LGTM!The
environmentin servicemigratoris correctly updated to include<<: [ *app-env, *db-env, *redis-env, *data-store-env, *proxy-env ], improving maintainability and clarity.The code changes are approved.
152-153: LGTM!The
environmentin serviceplane-dbis correctly updated to include<<: *db-env, improving maintainability and clarity.The code changes are approved.
161-162: LGTM!The
environmentin serviceplane-redisis correctly updated to include<<: *redis-env, improving maintainability and clarity.The code changes are approved.
171-172: LGTM!The
environmentin serviceplane-miniois correctly updated to include<<: *data-store-env, improving maintainability and clarity.The code changes are approved.
183-184: LGTM!The
portsin serviceproxyare correctly updated to useLISTEN_HTTP_PORTandLISTEN_HTTPS_PORTvariables, providing greater flexibility in configuring the service's network settings.The code changes are approved.
186-187: LGTM!The
volumesin serviceproxyare correctly updated to includecaddy_configandcaddy_data, indicating an expansion in the functionality of the proxy service.The code changes are approved.
188-189: LGTM!The
environmentin serviceproxyis correctly updated to include<<: *proxy-env, improving maintainability and clarity.The code changes are approved.
203-204: LGTM!The
volumessection is correctly updated to includecaddy_configandcaddy_data, indicating an expansion in the functionality of the proxy service.The code changes are approved.
.github/workflows/build-branch.yml (1)
333-334: LGTM!The
contextandfilepaths in thebranch_build_push_proxyjob are correctly updated to use thecaddydirectory, indicating a shift in the technology or configuration being utilized for the proxy service.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- caddy/caddy.sh (1 hunks)
- deploy/selfhost/docker-compose.yml (6 hunks)
- deploy/selfhost/variables.env (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- deploy/selfhost/variables.env
Additional context used
Shellcheck
caddy/caddy.sh
[warning] 3-3: In POSIX sh, == in place of = is undefined.
(SC3014)
[warning] 5-5: In POSIX sh, == in place of = is undefined.
(SC3014)
yamllint
deploy/selfhost/docker-compose.yml
[error] 204-204: no new line character at the end of file
(new-line-at-end-of-file)
[error] 204-204: trailing spaces
(trailing-spaces)
Additional comments not posted (3)
deploy/selfhost/docker-compose.yml (3)
1-8: Excellent use of YAML anchors for environment variables.The introduction of
x-db-env,x-redis-env,x-data-store-env, andx-proxy-envenhances modularity and reduces redundancy. This approach allows for cleaner and more maintainable Docker Compose configurations.The changes are approved.
Also applies to: 10-14, 15-25, 26-34
96-97: Proper usage of YAML anchors in service configurations.The environment variable declarations for services like
api,worker,beat-worker,migrator,plane-db,plane-redis,plane-minio, andproxycorrectly utilize the defined YAML anchors. This ensures that each service inherits the necessary environment variables without cluttering the main service definitions.The changes are approved.
Also applies to: 110-111, 125-126, 140-141, 151-152, 160-161, 170-171, 187-188
Line range hint
182-203: Dynamic port mapping and volume additions for the proxy service.The changes to port mapping and the addition of
caddy_configandcaddy_datavolumes for the proxy service are well-implemented, providing flexibility and supporting the management of SSL certificates and configuration files.The changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/variables.env (2 hunks)
Additional comments not posted (2)
deploy/selfhost/variables.env (2)
3-3: Review SSL default setting.The
SSL=falsesetting is appropriate for default configurations, ensuring that SSL is opt-in rather than enforced by default. This approach supports backward compatibility and flexibility for environments where SSL might not be necessary.The default setting is approved as it provides flexibility in deployment.
35-35: Review ACME CA URL setting.The
CERT_ACME_CA=https://acme-v02.api.letsencrypt.org/directoryis correctly set to point to the Let's Encrypt ACME directory. This setting is crucial for the automatic issuance and renewal of SSL certificates.The ACME CA URL is correctly configured for SSL certificate management.
deploy/selfhost/docker-compose.yml
Outdated
| SSL: ${SSL:-false} | ||
| APP_DOMAIN: ${APP_DOMAIN:-localhost} | ||
| FILE_SIZE_LIMIT: ${FILE_SIZE_LIMIT:-5242880} | ||
| CERT_EMAIL: ${CERT_EMAIL:-akshatjain9782@gmail.com} |
There was a problem hiding this comment.
Change it to CERT_EMAIL: ${CERT_EMAIL}
deploy/selfhost/docker-compose.yml
Outdated
| CERT_EMAIL: ${CERT_EMAIL:-akshatjain9782@gmail.com} | ||
| CERT_ACME_CA: ${CERT_ACME_CA:-} | ||
| LISTEN_HTTP_PORT: ${LISTEN_PORT:-80} | ||
| LISTEN_HTTPS_PORT: ${LISTEN_SSL_PORT} |
There was a problem hiding this comment.
Change this to LISTEN_HTTPS_PORT: ${LISTEN_SSL_PORT:-443}
deploy/selfhost/variables.env
Outdated
| REDIS_URL= | ||
|
|
||
| # If SSL Cert to be generated, set CERT_EMAIL and APP_PROTOCOL to https | ||
| CERT_EMAIL=demo@example.com |
There was a problem hiding this comment.
leave it empty without value CERT_EMAIL=
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/variables.env (2 hunks)
Additional comments not posted (4)
deploy/selfhost/variables.env (4)
3-3: SSL Default SettingThe
SSL=falsesetting is clear and indicates that SSL is not enabled by default. This is a safe default but ensure that documentation or deployment scripts clearly guide users on how to enable SSL if needed.
10-11: Review Server Port ConfigurationThe configuration includes both standard HTTP (
LISTEN_PORT=80) and HTTPS (LISTEN_SSL_PORT=443) ports. This setup is typical and appropriate for applications that may switch between SSL and non-SSL modes. Ensure firewall and security group settings are configured to appropriately expose these ports based on the environment.
35-35: Specify ACME CA for SSL CertificatesThe
CERT_ACME_CA=https://acme-v02.api.letsencrypt.org/directorysetting specifies the ACME directory URL for Let's Encrypt, which is standard for obtaining SSL certificates. This setting is correctly configured for environments that will use Let's Encrypt as the CA.
36-36: Review Trusted Proxies SettingSetting
TRUSTED_PROXIES=0.0.0.0/0allows all IP addresses to be trusted as proxies. This setting can introduce security risks if not properly managed, especially in a production environment. Consider restricting this setting to specific IP ranges or addresses that are known to be secure to mitigate potential security risks.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/docker-compose.yml (6 hunks)
Additional context used
yamllint
deploy/selfhost/docker-compose.yml
[error] 204-204: no new line character at the end of file
(new-line-at-end-of-file)
[error] 204-204: trailing spaces
(trailing-spaces)
Additional comments not posted (14)
deploy/selfhost/docker-compose.yml (14)
1-8: Well-defined PostgreSQL environment configuration.The anchor
x-db-envis well-structured and uses environment variables with sensible defaults, which enhances the configurability and isolation of PostgreSQL settings.
10-13: Well-defined Redis environment configuration.The anchor
x-redis-envis correctly set up with default values, facilitating better management of Redis settings.
15-24: Comprehensive MinIO environment configuration.The anchor
x-data-store-envincludes comprehensive settings for MinIO, including AWS compatibility, which enhances the flexibility and configurability of data storage settings.
26-33: Robust proxy environment configuration with SSL support.The anchor
x-proxy-envis crucial for the SSL feature added by this PR. It includes settings for SSL, domain, and port configurations, enhancing the security and flexibility of the proxy service.
34-46: Comprehensive application environment configuration.The anchor
x-app-envis set up with a variety of settings that are crucial for the application's operation, ensuring a robust configuration management.
96-97: Well-configured API service with comprehensive environment settings.The service
apiuses combined environment variables from all anchors, ensuring that it has all necessary configurations for robust operation.
110-111: Consistent environment configuration across backend services.The service
workermirrors theapiservice in using combined environment variables from all anchors, ensuring consistency and comprehensive settings.
125-126: Uniform environment configuration for scheduled tasks.The service
beat-workeruses combined environment variables from all anchors, ensuring uniformity and comprehensive settings for scheduled tasks.
140-141: Consistent environment configuration for migration tasks.The service
migratoruses combined environment variables from all anchors, ensuring consistency and comprehensive settings for migration tasks.
151-152: Properly configured database service.The service
plane-dbuses thedb-envanchor for environment variables, ensuring that it has all necessary PostgreSQL configurations for robust operation.
160-161: Well-configured Redis service.The service
plane-redisuses theredis-envanchor for environment variables, ensuring that it is properly configured for robust operation.
170-171: Properly configured MinIO service.The service
plane-miniouses thedata-store-envanchor for environment variables, ensuring that it has all necessary configurations for robust data storage operation.
182-188: Enhanced proxy service configuration with SSL support.The service
proxyuses theproxy-envanchor for environment variables, including SSL settings. The dynamic port mapping enhances the flexibility and security of the proxy service.
202-203: Appropriate addition of SSL-related volumes.The new volumes
caddy_configandcaddy_dataare appropriately added, likely for managing SSL certificates and configuration files for the proxy service.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .github/workflows/build-branch.yml (1 hunks)
- deploy/selfhost/install.sh (4 hunks)
Additional context used
Shellcheck
deploy/selfhost/install.sh
[warning] 238-238: Quote this to prevent word splitting.
(SC2046)
[warning] 241-241: Quote this to prevent word splitting.
(SC2046)
[warning] 248-248: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
GitHub Check: Codacy Static Code Analysis
deploy/selfhost/install.sh
[warning] 238-238: deploy/selfhost/install.sh#L238
Double quote to prevent globbing and word splitting.
[warning] 248-248: deploy/selfhost/install.sh#L248
Double quote to prevent globbing and word splitting.
Additional comments not posted (6)
.github/workflows/build-branch.yml (2)
333-334: LGTM! But verify the Dockerfile path.The change to use Caddy instead of Nginx for the proxy service looks good. Ensure that the new Dockerfile path is correct and properly configured.
The code changes are approved.
Run the following script to verify the Dockerfile path:
Verification successful
Dockerfile Path and Configuration Verified
The Caddy Dockerfile exists at the specified path and appears to be correctly configured for running a Caddy server. The
FROMstatement and setup commands are present and appropriate. No further issues are identified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the Caddy Dockerfile. # Test: Check if the Caddy Dockerfile exists. if [ -f "./caddy/Dockerfile" ]; then echo "Caddy Dockerfile exists." else echo "Caddy Dockerfile does not exist." exit 1 fiLength of output: 90
Script:
#!/bin/bash # Description: Check the contents of the Caddy Dockerfile for basic configuration. # Display the first few lines of the Dockerfile to check for a FROM statement. head -n 10 ./caddy/DockerfileLength of output: 213
343-390: LGTM! But verify the new job configuration.The addition of the
publish_assetsjob looks good. Ensure that the steps are correctly configured and that the assets are properly uploaded.The code changes are approved.
Run the following script to verify the new job configuration:
Verification successful
Verification Successful:
publish_assetsJob Configuration is CorrectThe
publish_assetsjob is correctly configured with the necessary steps and conditions for release events. The job includes checking out files, updating shell scripts, and uploading assets as expected.
- The job runs on
ubuntu-latestand depends on other build jobs.- It includes steps to update shell scripts and upload assets like
docker-compose.yaml,variables.env, andinstall.sh.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the configuration of the `publish_assets` job. # Test: Check if the steps in the `publish_assets` job are correctly configured. rg --type yaml -A 10 $'publish_assets:\n if : ${{ github.event_name == \'release\' }}\n runs-on: ubuntu-latest\n needs: [branch_build_push_web, branch_build_push_admin, branch_build_push_space, branch_build_push_apiserver, branch_build_push_proxy]\n steps:\n - id: checkout_files\n name: Checkout Files\n uses: actions/checkout@v4\n - name: Update shell script branch\n run: |\n sed -i "s/^RELEASE_TAG=.*/RELEASE_TAG=${{ github.event.release.tag_name }}/" ./deploy/selfhost/install.sh\n sed -i "s/^export APP_RELEASE=.*/export APP_RELEASE=${{ github.event.release.tag_name }}/g" ./deploy/selfhost/install.sh\n sed -i "s/APP_RELEASE=stable/APP_RELEASE=${{ github.event.release.tag_name }}/g" ./deploy/selfhost/install.sh\n - name: Upload docker-compose.yaml\n uses: actions/upload-release-asset@v1.0.1\n env:\n GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}\n with:\n upload_url: ${{ github.event.release.upload_url }}\n asset_path: ./deploy/selfhost/docker-compose.yml\n asset_name: docker-compose.yaml\n asset_content_type: application/octet-stream\n - name: Upload variables.env\n uses: actions/upload-release-asset@v1.0.1\n env:\n GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}\n with:\n upload_url: ${{ github.event.release.upload_url }}\n asset_path: ./deploy/selfhost/variables.env\n asset_name: variables.env\n asset_content_type: application/octet-stream\n - name: Upload setup.sh\n uses: actions/upload-release-asset@v1.0.1\n env:\n GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}\n with:\n upload_url: ${{ github.event.release.upload_url }}\n asset_path: ./deploy/selfhost/install.sh\n asset_name: setup.sh\n asset_content_type: application/octet-stream'Length of output: 4015
Script:
#!/bin/bash # Description: Verify the configuration of the `publish_assets` job using a simpler pattern. # Test: Check if the `publish_assets` job is defined and includes essential steps. rg --type yaml -A 20 'publish_assets:' .github/workflows/build-branch.ymlLength of output: 1132
deploy/selfhost/install.sh (4)
4-4: LGTM!The introduction of the
RELEASE_TAGvariable improves the script's functionality by making it more adaptable to versioning.The code changes are approved.
170-171: LGTM!The modification to replace
APP_RELEASEwith the latest value ensures that the variable is always up-to-date.The code changes are approved.
238-256: LGTM!The modification to include the
RELEASE_TAGvariable in the download URLs enhances the robustness of the file retrieval process.The code changes are approved.
Tools
Shellcheck
[warning] 238-238: Quote this to prevent word splitting.
(SC2046)
[warning] 241-241: Quote this to prevent word splitting.
(SC2046)
[warning] 248-248: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
GitHub Check: Codacy Static Code Analysis
[warning] 238-238: deploy/selfhost/install.sh#L238
Double quote to prevent globbing and word splitting.
[warning] 248-248: deploy/selfhost/install.sh#L248
Double quote to prevent globbing and word splitting.
362-362: LGTM!The modification to include the
APP_RELEASEvariable in the download message improves clarity for the user regarding which version is being processed.The code changes are approved.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/build.yml (1 hunks)
Additional comments not posted (1)
deploy/selfhost/build.yml (1)
29-29: LGTM! But verify the new build context and Dockerfile.The change in build context from
./nginxto./caddyindicates a transition to using Caddy as the web server. Ensure that the new build context and Dockerfile are correctly set up for Caddy.The code changes are approved.
Run the following script to verify the existence and correctness of the new build context and Dockerfile:
Verification successful
Verification Successful: Caddy Build Context and Dockerfile Exist
The
caddydirectory and itsDockerfileare present, confirming that the build context change is correctly set up for Caddy. No further issues are found with the transition from Nginx to Caddy.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of the new build context and Dockerfile for Caddy. # Test: Check if the `caddy` directory exists. Expect: Directory exists. if [ -d "./caddy" ]; then echo "Caddy directory exists." else echo "Caddy directory does not exist." exit 1 fi # Test: Check if the `Dockerfile` exists in the `caddy` directory. Expect: File exists. if [ -f "./caddy/Dockerfile" ]; then echo "Dockerfile exists in the caddy directory." else echo "Dockerfile does not exist in the caddy directory." exit 1 fiLength of output: 202
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/install.sh (4 hunks)
Additional context used
Shellcheck
deploy/selfhost/install.sh
[warning] 238-238: Quote this to prevent word splitting.
(SC2046)
[warning] 241-241: Quote this to prevent word splitting.
(SC2046)
[warning] 248-248: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
Additional comments not posted (4)
deploy/selfhost/install.sh (4)
4-4: LGTM!The introduction of the
RELEASE_TAGvariable with a default value ofv0.22-devis a good enhancement for version management.
170-171: LGTM!The update to replace
APP_RELEASEwith the latest value in thesyncEnvFilefunction ensures that the environment variable is synchronized correctly.
176-176: LGTM!The introduction of the
buildYourOwnImagefunction to build Docker images locally is a useful addition for custom builds.
362-362: LGTM!The update to download the specified
APP_RELEASEversion in theupgradefunction ensures that the correct version is downloaded during the upgrade process.
deploy/selfhost/install.sh
Outdated
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/docker-compose.yaml?$(date +%s) | ||
| if [ $? -ne 0 ]; then | ||
| # 2nd attempt to download the file for backward compatibility | ||
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s) | ||
| if [ $? -ne 0 ]; then | ||
| echo "Failed to download the docker-compose.yaml file. Exiting..." | ||
| exit 1 | ||
| fi | ||
| fi | ||
|
|
||
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/variables.env?$(date +%s) | ||
| if [ $? -ne 0 ]; then | ||
| # 2nd attempt to download the file for backward compatibility | ||
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s) | ||
| if [ $? -ne 0 ]; then | ||
| echo "Failed to download the variables.env file. Exiting..." | ||
| exit 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
LGTM! But fix potential word splitting issues.
The update to use the RELEASE_TAG variable for downloading files improves the download logic. However, there are potential issues with word splitting that need to be addressed.
Apply this diff to fix the word splitting issues:
- curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/docker-compose.yaml?$(date +%s)
+ curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/docker-compose.yaml" "https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/docker-compose.yaml?$(date +%s)"
- curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s)
+ curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/docker-compose.yaml" "https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s)"
- curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/variables.env?$(date +%s)
+ curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/variables-upgrade.env" "https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/variables.env?$(date +%s)"
- curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s)
+ curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/variables-upgrade.env" "https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s)"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.
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/docker-compose.yaml?$(date +%s) | |
| if [ $? -ne 0 ]; then | |
| # 2nd attempt to download the file for backward compatibility | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s) | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to download the docker-compose.yaml file. Exiting..." | |
| exit 1 | |
| fi | |
| fi | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/variables.env?$(date +%s) | |
| if [ $? -ne 0 ]; then | |
| # 2nd attempt to download the file for backward compatibility | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s) | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to download the variables.env file. Exiting..." | |
| exit 1 | |
| fi | |
| fi | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/docker-compose.yaml" "https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/docker-compose.yaml?$(date +%s)" | |
| if [ $? -ne 0 ]; then | |
| # 2nd attempt to download the file for backward compatibility | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/docker-compose.yaml" "https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s)" | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to download the docker-compose.yaml file. Exiting..." | |
| exit 1 | |
| fi | |
| fi | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/variables-upgrade.env" "https://github.com/makeplane/plane/releases/download/$RELEASE_TAG/variables.env?$(date +%s)" | |
| if [ $? -ne 0 ]; then | |
| # 2nd attempt to download the file for backward compatibility | |
| curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/variables-upgrade.env" "https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s)" | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to download the variables.env file. Exiting..." | |
| exit 1 | |
| fi | |
| fi |
Tools
Shellcheck
[warning] 238-238: Quote this to prevent word splitting.
(SC2046)
[warning] 241-241: Quote this to prevent word splitting.
(SC2046)
[warning] 248-248: Quote this to prevent word splitting.
(SC2046)
[warning] 251-251: Quote this to prevent word splitting.
(SC2046)
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
deploy/selfhost/README.md (1)
109-117: Configuration options updated for clarity.The updates to the configuration options, including
APP_DOMAIN,LISTEN_PORT,LISTEN_SSL_PORT,WEB_URL, andCORS_ALLOWED_ORIGINS, improve the clarity of the instructions and provide users with more detailed guidance on setting up their environment.Please update the abbreviation "e.g." to use two periods, as suggested by the LanguageTool hints:
- (eg. `plane.example.com`) + (e.g., `plane.example.com`) - (eg. `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`) + (e.g., `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`) - (eg. `http://plane.example.com:8080,https://plane.example.com:8443`) + (e.g., `http://plane.example.com:8080,https://plane.example.com:8443`)The Markdownlint hints regarding blank lines inside blockquotes can be ignored as they are false positives in this context.
Tools
LanguageTool
[uncategorized] ~109-~109: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... the Fully Qualified Domain Name here. (eg.plane.example.com) >LISTEN_PORT-...(E_G)
[uncategorized] ~115-~115: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...along with LISTEN_PORT/LISTEN_SSL_PORT (eg.https://plane.example.com:8443or `ht...(E_G)
[uncategorized] ~117-~117: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://...(E_G)
Markdownlint
110-110: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
112-112: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
114-114: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
116-116: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/build-branch.yml (1 hunks)
- deploy/selfhost/README.md (9 hunks)
- deploy/selfhost/variables.env (2 hunks)
Additional context used
Markdownlint
deploy/selfhost/README.md
60-60: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
110-110: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
112-112: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
114-114: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
116-116: null
Blank line inside blockquote(MD028, no-blanks-blockquote)
LanguageTool
deploy/selfhost/README.md
[uncategorized] ~109-~109: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... the Fully Qualified Domain Name here. (eg.plane.example.com) >LISTEN_PORT-...(E_G)
[uncategorized] ~115-~115: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...along with LISTEN_PORT/LISTEN_SSL_PORT (eg.https://plane.example.com:8443or `ht...(E_G)
[uncategorized] ~117-~117: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://...(E_G)
Additional comments not posted (10)
deploy/selfhost/variables.env (4)
3-3: LGTM!The code changes are approved.
10-10: LGTM!The code changes are approved.
11-11: LGTM!The code changes are approved.
16-16: LGTM!The code changes are approved.
.github/workflows/build-branch.yml (2)
333-334: LGTM!The change from Nginx to Caddy aligns with the PR objective of adding SSL support. Caddy's automatic HTTPS capabilities will enhance the security of the deployed services.
343-400: LGTM!The new
publish_assetsjob is a great addition to the workflow. It automates the asset publishing process during releases, which will improve deployment efficiency and ensure that the correct versioned assets are available.deploy/selfhost/README.md (4)
58-58: LGTM!The change in the section header from "Latest Stable Release" to "Latest Release" is approved as it reflects the shift in focus to the latest version available.
61-61: LGTM!The addition of the
-pflag to themkdircommand is approved as it ensures that the directory is created only if it does not already exist, enhancing robustness.
63-63: LGTM!The introduction of the
RELEASE_VERSIONvariable is approved as it allows for a more flexible approach to version specification, replacing the hardcoded URL.
65-65: LGTM!The update to the curl command to use the
RELEASE_VERSIONvariable is approved as it ensures that the correct version of the setup script is downloaded based on the specified version.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/README.md (9 hunks)
Additional context used
Markdownlint
deploy/selfhost/README.md
60-60: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
LanguageTool
deploy/selfhost/README.md
[uncategorized] ~109-~109: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... the Fully Qualified Domain Name here. (eg.plane.example.com) > >LISTEN_PORT...(E_G)
[uncategorized] ~115-~115: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...along with LISTEN_PORT/LISTEN_SSL_PORT (eg.https://plane.example.com:8443or `ht...(E_G)
[uncategorized] ~117-~117: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://...(E_G)
Additional comments not posted (9)
deploy/selfhost/README.md (9)
58-58: LGTM!The header text change is approved.
61-61: LGTM!The addition of the
-pflag to themkdircommand is approved.
63-63: LGTM!The introduction of the
RELEASE_VERSIONvariable is approved.
65-65: LGTM!The update to the URL for downloading
setup.shis approved.
141-141: LGTM!The addition of the comment is approved.
176-176: LGTM!The addition of the comment is approved.
204-204: LGTM!The addition of the comment is approved.
232-232: LGTM!The addition of the comment is approved.
Line range hint
359-382: LGTM!The changes to the data restoration section are approved.
| > `APP_DOMAIN` - Set the Fully Qualified Domain Name here. (eg. `plane.example.com`) | ||
| > | ||
| > `LISTEN_PORT` - This is default set to `80`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_PORT=8080`) | ||
| > | ||
| > `LISTEN_SSL_PORT` - This is default set to `443`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_SSL_PORT=8443`) | ||
| > | ||
| > `WEB_URL` - This is default set to `http://localhost`. Change this to the FQDN you plan to use along with LISTEN_PORT/LISTEN_SSL_PORT (eg. `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`) | ||
| > | ||
| > `CORS_ALLOWED_ORIGINS` - This is default set to `http://${APP_DOMAIN},https://${APP_DOMAIN}`. Change this to the FQDN you plan to use along with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://plane.example.com:8443`) |
There was a problem hiding this comment.
LGTM!
The changes to the configuration options are approved.
Fix the abbreviation for "for example".
The explanations use "eg." instead of "e.g." for the abbreviation "for example". Please update the occurrences of "eg." to "e.g.".
Apply this diff to fix the issue:
- (eg. `plane.example.com`)
+ (e.g., `plane.example.com`)
- (eg. `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`)
+ (e.g., `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`)
- (eg. `http://plane.example.com:8080,https://plane.example.com:8443`)
+ (e.g., `http://plane.example.com:8080,https://plane.example.com:8443`)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.
| > `APP_DOMAIN` - Set the Fully Qualified Domain Name here. (eg. `plane.example.com`) | |
| > | |
| > `LISTEN_PORT` - This is default set to `80`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_PORT=8080`) | |
| > | |
| > `LISTEN_SSL_PORT` - This is default set to `443`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_SSL_PORT=8443`) | |
| > | |
| > `WEB_URL` - This is default set to `http://localhost`. Change this to the FQDN you plan to use along with LISTEN_PORT/LISTEN_SSL_PORT (eg. `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`) | |
| > | |
| > `CORS_ALLOWED_ORIGINS` - This is default set to `http://${APP_DOMAIN},https://${APP_DOMAIN}`. Change this to the FQDN you plan to use along with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://plane.example.com:8443`) | |
| > `APP_DOMAIN` - Set the Fully Qualified Domain Name here. (e.g., `plane.example.com`) | |
| > | |
| > `LISTEN_PORT` - This is default set to `80`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_PORT=8080`) | |
| > | |
| > `LISTEN_SSL_PORT` - This is default set to `443`. Make sure the port you choose to use is not preoccupied. (e.g `LISTEN_SSL_PORT=8443`) | |
| > | |
| > `WEB_URL` - This is default set to `http://localhost`. Change this to the FQDN you plan to use along with LISTEN_PORT/LISTEN_SSL_PORT (e.g., `https://plane.example.com:8443` or `http://[IP-ADDRESS]:8080`) | |
| > | |
| > `CORS_ALLOWED_ORIGINS` - This is default set to `http://${APP_DOMAIN},https://${APP_DOMAIN}`. Change this to the FQDN you plan to use along with LISTEN_PORT and LISTEN_SSL_PORT (e.g., `http://plane.example.com:8080,https://plane.example.com:8443`) |
Tools
LanguageTool
[uncategorized] ~109-~109: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... the Fully Qualified Domain Name here. (eg.plane.example.com) > >LISTEN_PORT...(E_G)
[uncategorized] ~115-~115: The abbreviation “e.g.” (= for example) requires two periods.
Context: ...along with LISTEN_PORT/LISTEN_SSL_PORT (eg.https://plane.example.com:8443or `ht...(E_G)
[uncategorized] ~117-~117: The abbreviation “e.g.” (= for example) requires two periods.
Context: ... with LISTEN_PORT and LISTEN_SSL_PORT (eg. `http://plane.example.com:8080,https://...(E_G)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- .github/workflows/build-branch.yml (1 hunks)
- aio/Dockerfile-app (3 hunks)
- aio/nginx.conf (1 hunks)
- aio/supervisord-app (1 hunks)
- caddy/Caddyfile.template (1 hunks)
- deploy/selfhost/build.yml (2 hunks)
- deploy/selfhost/docker-compose.yml (6 hunks)
- deploy/selfhost/variables.env (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-branch.yml
- caddy/Caddyfile.template
Additional context used
yamllint
deploy/selfhost/docker-compose.yml
[error] 19-19: trailing spaces
(trailing-spaces)
Additional comments not posted (17)
deploy/selfhost/build.yml (2)
20-24: LGTM!The new
liveservice configuration looks good. Using a dedicated Dockerfile for the live environment is a nice way to customize the build for that specific environment.
35-35: Verify the Caddy configuration.The change in the build context for the
proxyservice looks good. Using Caddy instead of Nginx aligns with the PR objective of adding SSL support.Please ensure that:
- The Caddy configuration file (Caddyfile) is properly set up with the required directives for SSL and reverse proxy.
- The SSL certificates are correctly provisioned and renewed.
- The Caddy configuration is thoroughly tested to ensure that SSL is working as expected and the application is accessible via HTTPS.
deploy/selfhost/variables.env (3)
3-3: LGTM!The code changes are approved.
35-35: LGTM!The code changes are approved.
16-16: LGTM!The code changes are approved.
aio/supervisord-app (1)
32-40: LGTM!The new program configuration
[program:live]looks good:
- It follows the same structure as the existing configurations, which is good for consistency.
- The logging configuration is set to log to
/dev/stdoutwith no size limits, which is consistent with the existing configurations and is a good practice for containerized applications.- The environment variables are defined correctly, including a new port (3004) and an API base URL pointing to a local server, allowing the live server to run on a different port and communicate with the API server.
aio/nginx.conf (1)
48-54: LGTM!The new location block for
/live/is correctly configured for handling WebSocket connections.
- The
proxy_http_versiondirective is set to 1.1, which is required for WebSocket connections.- The
Upgrade,Connection, andHostheaders are correctly set for WebSocket connections.- The
proxy_passdirective is correctly set to forward requests to the new backend service running onhttp://localhost:3004/.The code changes are approved.
aio/Dockerfile-app (3)
14-14: LGTM!The addition of
--scope=liveto theturbo prunecommand ensures that the "live" component is considered during the pruning phase of the build process.
56-56: LGTM!The addition of
--filter=liveto theyarn turbo run buildcommand ensures that the "live" component is built alongside the existing components.
108-110: LGTM!The added
COPYcommands ensure that the necessary files and directories for the "live" component are included in the final Docker image.deploy/selfhost/docker-compose.yml (7)
1-8: LGTM!The
x-db-envanchor is a good addition for organizing the PostgreSQL environment variables.
10-13: LGTM!The
x-redis-envanchor is a good addition for organizing the Redis environment variables.
20-30: LGTM!The
x-data-store-envanchor is a good addition for organizing the MinIO environment variables.
31-39: LGTM!The
x-proxy-envanchor is a good addition for organizing the proxy environment variables.
106-107: LGTM!The environment variable declarations for the services have been improved by using the newly introduced anchors. This enhances the organization and maintainability of the configuration.
Also applies to: 124-125, 139-140, 155-156, 171-172, 182-183, 191-192, 209-210, 226-227
221-222: LGTM!The port mapping for the proxy service has been improved by using environment variables. This provides flexibility in configuring the ports.
241-242: Verify the purpose of the new volumes.The new volumes
caddy_configandcaddy_datahave been added for the proxy service. Can you please provide more context on the purpose of these volumes and how they are being used by the Caddy web server?
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- aio/Dockerfile-app (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- aio/Dockerfile-app
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
deploy/selfhost/variables.env (1)
3-3: LGTM, but ensure SSL is enabled in production.The new
SSLvariable, set tofalseby default, is suitable for local development environments.However, for production deployments, it is crucial to set
SSL=trueand configure a valid SSL certificate to ensure secure connections.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- .github/workflows/build-branch.yml (1 hunks)
- deploy/selfhost/docker-compose.yml (6 hunks)
- deploy/selfhost/variables.env (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-branch.yml
Additional context used
yamllint
deploy/selfhost/docker-compose.yml
[error] 19-19: trailing spaces
(trailing-spaces)
Additional comments not posted (16)
deploy/selfhost/variables.env (2)
36-36: LGTM!The
CERT_ACME_CAvariable is set to the correct URL for obtaining SSL certificates from Let's Encrypt.
16-16: LGTM!The update to the
CORS_ALLOWED_ORIGINSvariable to include both HTTP and HTTPS origins is a good change to support environments with SSL enabled or disabled.deploy/selfhost/docker-compose.yml (14)
1-9: LGTM!The introduction of the
x-db-envanchor for PostgreSQL environment variables is a good practice for modular configuration management.
10-13: LGTM!The introduction of the
x-redis-envanchor for Redis environment variables is a good practice for modular configuration management.
15-18: LGTM!The introduction of the
x-mq-envanchor for RabbitMQ environment variables is a good practice for modular configuration management.
20-29: LGTM!The introduction of the
x-data-store-envanchor for MinIO environment variables is a good practice for modular configuration management.
31-38: LGTM!The introduction of the
x-proxy-envanchor for proxy environment variables is a good practice for modular configuration management.
40-41: LGTM!The introduction of the
x-live-envanchor for live service environment variables is a good practice for modular configuration management.
106-107: LGTM!The update to the
environmentsection of theliveservice to include thex-live-envandx-redis-envanchors is a good change for consolidating environment variable management.
124-125: LGTM!The update to the
environmentsection of theapiservice to include multiple environment variable anchors is a good change for consolidating environment variable management.
139-140: LGTM!The update to the
environmentsection of theworkerservice to include multiple environment variable anchors is a good change for consolidating environment variable management.
155-156: LGTM!The update to the
environmentsection of thebeat-workerservice to include multiple environment variable anchors is a good change for consolidating environment variable management.
171-172: LGTM!The update to the
environmentsection of themigratorservice to include multiple environment variable anchors is a good change for consolidating environment variable management.
221-222: LGTM!The update to the port mapping for the
proxyservice to use dynamic port definitions provides greater flexibility in configuring the service's network settings.
223-225: LGTM!The addition of the
caddy_configandcaddy_datavolumes for theproxyservice is a good change to support the management of SSL certificates and configuration files.
19-19: Remove trailing spaces.The static analysis tool yamllint has flagged a trailing spaces issue at line 19.
Trailing spaces can cause issues with some tools and should be removed for consistency and cleanliness of the code.
Tools
yamllint
[error] 19-19: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/selfhost/install.sh (4 hunks)
Additional context used
Shellcheck
deploy/selfhost/install.sh
[warning] 244-244: Quote this to prevent word splitting.
(SC2046)
Additional comments not posted (4)
deploy/selfhost/install.sh (4)
4-4: LGTM!The
RELEASE_TAGvariable declaration with a default value provides flexibility to manage different release versions.
170-171: LGTM!The update to replace
APP_RELEASEwith the latest value ensures that the deployment reflects the correct application release version and improves the synchronization of environment variables.
239-242: LGTM!The error handling after
curlcommands prevents the script from proceeding with potentially incomplete or missing configurations, thereby increasing the reliability of the installation process.Also applies to: 245-248
354-354: LGTM!Dynamically displaying the current
APP_RELEASEversion provides clearer feedback to the user regarding the version being downloaded during the upgrade process.
deploy/selfhost/install.sh
Outdated
| exit 1 | ||
| fi | ||
|
|
||
| curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/variables-upgrade.env https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/variables.env?$(date +%s) |
There was a problem hiding this comment.
Fix the potential word splitting issue.
The variable expansion at line 244 should be quoted to prevent potential word splitting issues.
Apply this diff to fix the issue:
- curl -H 'Cache-Control: no-cache, no-store' -s -o $PLANE_INSTALL_DIR/docker-compose.yaml https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s)
+ curl -H 'Cache-Control: no-cache, no-store' -s -o "$PLANE_INSTALL_DIR/docker-compose.yaml" "https://raw.githubusercontent.com/makeplane/plane/$BRANCH/deploy/selfhost/docker-compose.yml?$(date +%s)"Committable suggestion was skipped due to low confidence.
Tools
Shellcheck
[warning] 244-244: Quote this to prevent word splitting.
(SC2046)
akshat5302
changed the title
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
deploy/selfhost/docker-compose.yml (1)
254-255: Ensure newline at end-of-file and strip trailing spaces.The same formatting errors (
new-line-at-end-of-file,trailing-spaces) are still present. Please add a final newline and remove any trailing whitespace to satisfyyamllint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
deploy/selfhost/docker-compose.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
| SSL: ${SSL:-false} | ||
| APP_DOMAIN: ${APP_DOMAIN:-localhost} | ||
| FILE_SIZE_LIMIT: ${FILE_SIZE_LIMIT:-5242880} | ||
| CERT_EMAIL: ${CERT_EMAIL:-admin@example.com} | ||
| CERT_ACME_CA: ${CERT_ACME_CA:-} | ||
| LISTEN_HTTP_PORT: ${LISTEN_PORT:-80} | ||
| LISTEN_HTTPS_PORT: ${LISTEN_SSL_PORT:-443} | ||
| BUCKET_NAME: ${AWS_S3_BUCKET_NAME:-uploads} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Separate proxy-only env variables from application env groups.
The x-proxy-env block currently mixes SSL/Caddy settings with upload limits and bucket names, then injects all of them into API/worker services. To avoid leaking proxy-specific vars into your backend and improve clarity, split this into two groups:
-
x-upload-env:- FILE_SIZE_LIMIT
- BUCKET_NAME
-
x-proxy-env:- SSL
- APP_DOMAIN
- CERT_EMAIL
- CERT_ACME_CA
- LISTEN_HTTP_PORT
- LISTEN_HTTPS_PORT
Use *upload-env in your API/worker services and reserve *proxy-env for the proxy service only.
🤖 Prompt for AI Agents
In deploy/selfhost/docker-compose.yml around lines 27 to 34, the environment
variables for proxy settings and upload settings are mixed in the same group.
Separate them into two distinct groups: create an x-upload-env group containing
FILE_SIZE_LIMIT and BUCKET_NAME, and an x-proxy-env group containing SSL,
APP_DOMAIN, CERT_EMAIL, CERT_ACME_CA, LISTEN_HTTP_PORT, and LISTEN_HTTPS_PORT.
Then, update the service definitions so that API and worker services use the
x-upload-env group, while the proxy service uses the x-proxy-env group to avoid
leaking proxy-specific variables into backend services.
| mode: host | ||
| environment: | ||
| <<: *proxy-env | ||
| image: artifacts.plane.so/makeplane/plane-proxy:${APP_RELEASE_VERSION} |
There was a problem hiding this comment.
Add a fallback default for ${APP_RELEASE_VERSION}.
If APP_RELEASE_VERSION isn’t set, the image tag will be empty and the pull will fail. Provide a default, for example:
-image: artifacts.plane.so/makeplane/plane-proxy:${APP_RELEASE_VERSION}
+image: artifacts.plane.so/makeplane/plane-proxy:${APP_RELEASE_VERSION:-stable}📝 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.
| image: artifacts.plane.so/makeplane/plane-proxy:${APP_RELEASE_VERSION} | |
| image: artifacts.plane.so/makeplane/plane-proxy:${APP_RELEASE_VERSION:-stable} |
🤖 Prompt for AI Agents
In deploy/selfhost/docker-compose.yml at line 220, the image tag uses
${APP_RELEASE_VERSION} without a fallback, which can cause pull failures if the
variable is unset. Modify the image tag to include a default value using shell
parameter expansion syntax, such as ${APP_RELEASE_VERSION:-default_version}, to
ensure a valid tag is always used.
2 participants
Changes:
This PR includes seprated env variables for specific app containers and added SSL feature using caddy
Summary by CodeRabbit
New Features
Improvements
Chores