chore: Add script to upgrade Postgres 13 data to 14#34317
Conversation
|
Warning Rate limit exceeded@sharat87 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 48 minutes and 23 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe updates enhance PostgreSQL support by upgrading from version 13 to 14 in the Docker environment, alongside improving the initialization and upgrade processes. Key changes include path updates, automation scripts for PostgreSQL upgrades, and test scripts to ensure successful upgrades. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Docker Container
participant Entrypoint Script
participant PG Upgrade Script
User->>Docker Container: Start Container
Docker Container->>Entrypoint Script: Execute entrypoint.sh
Entrypoint Script->>PG Upgrade Script: Check PG_VERSION, run pg-upgrade.sh if exists
PG Upgrade Script->>PostgreSQL: Perform Upgrade (check version, data migration)
PG Upgrade Script->>Entrypoint Script: Complete Upgrade
Entrypoint Script->>Docker Container: Continue Initialization
Docker Container->>User: Container Ready with PostgreSQL 14
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9575843921. |
|
Deploy-Preview-URL: https://ce-34317.dp.appsmith.com |
sharat87
added
ok-to-test
This reverts commit 65d90fe.
sharat87
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (3)
deploy/docker/tests/pg-upgrade/run.sh (2)
3-3: Consider removing the "WIP" tag if the script is ready for production.
18-18: Clarify the TODO comment about--pull alwaysor remove it if no longer relevant.deploy/docker/fs/opt/appsmith/pg-upgrade.sh (1)
6-19: Ensure that the script's assumptions and contract are clearly documented and validated at runtime.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- deploy/docker/base.dockerfile (1 hunks)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (2 hunks)
- deploy/docker/fs/opt/appsmith/pg-upgrade.sh (1 hunks)
- deploy/docker/tests/pg-upgrade/run.sh (1 hunks)
Files not reviewed due to errors (1)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (no review received)
Additional context used
Shellcheck
deploy/docker/tests/pg-upgrade/run.sh
[error] 87-87: Couldn't parse this test expression. Fix to allow more checks. (SC1073)
[error] 87-87: Expected test to end here (don't wrap commands in []/[[]]). Fix any mentioned problems and try again. (SC1072)
deploy/docker/fs/opt/appsmith/entrypoint.sh
[warning] 62-62: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 66-66: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 74-74: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 86-86: ShellCheck can't follow non-constant source. Use a directive to specify location. (SC1090)
Hadolint
deploy/docker/base.dockerfile
[warning] 17-17: Pin versions in apt get install. Instead of
apt-get install <package>useapt-get install <package>=<version>(DL3008)
[info] 17-17: Delete the apt-get lists after installing something (DL3009)
[warning] 17-17: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006)
[warning] 17-17: Do not use apt as it is meant to be a end-user tool, use apt-get or apt-cache instead (DL3027)
[warning] 38-38: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006)
[info] 38-38: Double quote to prevent globbing and word splitting. (SC2086)
[warning] 49-49: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006)
[warning] 55-55: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check (DL4006)
Additional comments not posted (4)
deploy/docker/tests/pg-upgrade/run.sh (1)
91-96: Ensure that the cleanup process does not inadvertently remove necessary resources.Verification successful
The cleanup commands
docker rm --force "$container_name"anddocker volume rm --force "$container_name"are used within a specific test script (deploy/docker/tests/pg-upgrade/run.sh). These commands are intended to remove the test container and its associated volume, which is a common practice to ensure a clean state for subsequent tests.
docker rm -f "$container_name": Removes the container forcefully.docker volume rm --force "$container_name": Removes the volume forcefully.Given that these commands are used in a controlled test environment and are specifically targeting a test container and volume (
appsmith-pg-upgrade-test), there is no indication that they would inadvertently remove necessary resources outside the scope of the test.
- Lines in
deploy/docker/tests/pg-upgrade/run.sh:
docker rm --force "$container_name"docker volume rm --force "$container_name"Therefore, the cleanup process appears to be safe and does not indicate any removal of necessary resources beyond the test scope.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential issues in the cleanup process. # Test: Search for patterns that might indicate risky cleanup operations. rg --type bash $'docker rm --force|docker volume rm --force'Length of output: 90
Script:
#!/bin/bash # Description: Check for potential issues in the cleanup process. # Test: Search for patterns that might indicate risky cleanup operations. rg 'docker rm --force|docker volume rm --force'Length of output: 446
Script:
#!/bin/bash # Description: Extract and review the context of cleanup commands in the specified script. # Extract lines around the identified commands for context. rg -C 5 'docker rm --force|docker volume rm --force' deploy/docker/tests/pg-upgrade/run.shLength of output: 477
deploy/docker/base.dockerfile (1)
35-35: Ensure that the PATH environment variable correctly prioritizes the necessary PostgreSQL version.deploy/docker/fs/opt/appsmith/pg-upgrade.sh (2)
36-39: Ensure that the error message is clear and provides actionable steps if Postgres was not shutdown cleanly.
47-84: Review the upgrade logic to ensure that it handles different scenarios robustly, particularly around version checks and data migration.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
deploy/docker/tests/pg-upgrade/run.sh (3)
15-16: Consider adding error handling for Docker commands.Given the nature of this script, adding error handling for Docker commands such as
docker rm -fanddocker volume rm --forcecould prevent the script from failing silently if these commands do not execute as expected. This would make the script more robust, especially in environments where Docker resources might be in a varied state.
18-18: Clarify the TODO comment regarding--pull always.The TODO comment on line 18 is a bit unclear. It would be beneficial to expand on this comment to specify under what conditions or in which scenarios the
--pull alwaysoption would be beneficial. This will help future maintainers understand the intent and evaluate the necessity of this change.
72-72: Remove unused variable or clarify its usage.The variable
sample_table_contentsis set but never used within the script. If this variable is meant to be used externally, consider exporting it; otherwise, it should be removed to avoid confusion.Tools
Shellcheck
[warning] 72-72: sample_table_contents appears unused. Verify use (or export if used externally). (SC2034)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/docker/tests/pg-upgrade/run.sh (1 hunks)
Additional context used
Learnings (1)
deploy/docker/tests/pg-upgrade/run.sh (1)
User: sharat87 PR: appsmithorg/appsmith#34317 File: deploy/docker/tests/pg-upgrade/run.sh:37-52 Timestamp: 2024-06-20T02:20:27.587Z Learning: sharat87 indicated that explicit error handling for `supervisorctl` and `psql` is not a priority, as the assumption is that if these commands fail, the entire system is likely down. This reflects a high confidence in the stability of these components under normal conditions.
Shellcheck
deploy/docker/tests/pg-upgrade/run.sh
[warning] 72-72: sample_table_contents appears unused. Verify use (or export if used externally). (SC2034)
Additional comments not posted (2)
deploy/docker/tests/pg-upgrade/run.sh (2)
37-53: Ensure consistent error handling in the script block.The script block executed within the Docker container (lines 37-53) does not handle potential errors from commands like
supervisorctl stopandpsql. Considering the previous feedback and current practices, it might be beneficial to add checks after these commands to ensure they succeed before proceeding. This would prevent the script from continuing in an inconsistent state.Skipped due to learnings
User: sharat87 PR: appsmithorg/appsmith#34317 File: deploy/docker/tests/pg-upgrade/run.sh:37-52 Timestamp: 2024-06-20T02:20:27.587Z Learning: sharat87 indicated that explicit error handling for `supervisorctl` and `psql` is not a priority, as the assumption is that if these commands fail, the entire system is likely down. This reflects a high confidence in the stability of these components under normal conditions.
42-48: Verify SQL commands execution.This segment inserts sample data into the PostgreSQL database. It's crucial to ensure that these SQL commands execute successfully, especially in a testing script where data integrity is significant for subsequent tests.
2 participants
We're upgrading embedded Postgres from 13 to 14, and this PR includes a script to perform the upgrade of the data folder from v13 schema to v14 schema. This script temporarily installs Postgres 13, if not available, for the upgrade process, so will continue to work when and if we choose to remove
postgresql-13from the base image.Tested this manually as well, running an Appsmith with Postgres 13, executing some workflows via webhook, getting some run data generated, then upgrading Postgres with the script in this PR, and ensuring that the workflow run history is still there and visible on the UI exactly the same. It is.
No conflicts or additional changes needed on EE. All server and Cypress tests pass on EE.
/test sanity
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9590240540
Commit: 9c75da5
Cypress dashboard.
Tags:
@tag.SanitySummary by CodeRabbit
New Features
Improvements