-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: add sqlite support in the upstall script #9537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new configuration variable, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
a2aa298 to
cd41294
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
docker-compose/1_Auto_Upstall/noco.sh (3)
Line range hint
589-634: Unnecessary port mapping for MinIO when disabledThe Traefik service is configured to map port 9000 even when MinIO is not enabled. This could expose an unused port and potentially cause confusion or security concerns.
Conditionally include the port mapping for port 9000 only when MinIO is enabled:
ports: - "80:80" - "443:443" - - "9000:9000"Add the port mapping when MinIO is enabled:
if [ "${CONFIG_MINIO_ENABLED}" = "Y" ]; then cat >> "$compose_file" <<EOF - "9000:9000" EOF fi
456-456: Clarify default option in database selection promptThe prompt for selecting the database shows
[P/S]with a default of "P" for PostgreSQL. This could be unclear to users who may not know which letter corresponds to which database.Clarify the prompt to indicate the full names and the default option:
- CONFIG_POSTGRE_SQLITE=$(prompt "Select PostgreSQL or SQLite as your database [P/S]" "P") + CONFIG_POSTGRE_SQLITE=$(prompt "Select your database: PostgreSQL [P] or SQLite [S] (default: P)" "P")
Line range hint
12-24: Domain name validation missingIn the
get_user_inputsfunction, when the user provides a domain name, there is no validation to ensure it's correctly formatted. This could lead to issues with SSL configuration and accessing NocoDB.Add validation for the domain name input:
CONFIG_DOMAIN_NAME=$(prompt "Enter the IP address or domain name for the NocoDB instance" "$(get_public_ip)") + while ! is_valid_domain "$CONFIG_DOMAIN_NAME"; do + print_error "The domain name '$CONFIG_DOMAIN_NAME' is not valid." + CONFIG_DOMAIN_NAME=$(prompt "Please enter a valid domain name" "$(get_public_ip)") + done
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- docker-compose/1_Auto_Upstall/noco.sh (8 hunks)
🔇 Additional comments not posted (1)
docker-compose/1_Auto_Upstall/noco.sh (1)
531-541:⚠️ Potential issueVariable 'gen_redis' checked twice in condition
In the
create_docker_compose_filefunction, the condition[ -n "$gen_redis" ] || [ "$gen_redis" ]repeats the check forgen_redis. This duplication is unnecessary.Simplify the condition by removing the duplicate check:
- if [ -n "$gen_postgres" ] || [ -n "$gen_redis" ]; then + if [ -n "$gen_postgres" ] || [ -n "$gen_redis" ]; thenLikely invalid or redundant comment.
97fe4cd to
8637c53
Compare
8637c53 to
6a20f3f
Compare
239bffd to
7b2fbad
Compare
7b2fbad to
7802557
Compare
7802557 to
84eb5e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not part of this pr, let's add -v while removing docker containers to remove any attached volume
$CONFIG_DOCKER_COMMAND compose down
to
$CONFIG_DOCKER_COMMAND compose down -v
pranavxc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes looks fine, will merge after couple more testing.
pranavxc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
706ec20 to
0f9db69
Compare
6479964 to
bf06cba
Compare
bf06cba to
938523d
Compare


Change Summary
Change type