chore(nix/docker/aio): release #11475
Conversation
📝 Walkthrough""" WalkthroughThis update restructures the NocoDB all-in-one Docker environment, introducing distinct main and worker services with separate logging and environment configurations. It changes image naming conventions, modifies service ports and Nginx routing, migrates legacy data, modularizes environment processing, replaces valkey with Redis, upgrades PostgreSQL to version 16, and revises multi-architecture Docker image building and manifest pushing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DockerContainer
participant S6Init
participant RedisService
participant MainService
participant WorkerService
participant Nginx
User->>DockerContainer: Start container
DockerContainer->>S6Init: Launch init process
S6Init->>RedisService: Start Redis service
S6Init->>MainService: Start NocoDB main service (port 8008)
S6Init->>WorkerService: Start NocoDB worker service (port 8007)
S6Init->>MainService: Pass environment from common/main env files
S6Init->>WorkerService: Pass environment from common/worker env files
S6Init->>MainService: Handle legacy data migration (if needed)
MainService-->>S6Init: Service logs to nocodb-main-log
WorkerService-->>S6Init: Service logs to nocodb-worker-log
RedisService-->>S6Init: Service logs to redis-log
Nginx->>MainService: Proxy traffic to 8008 (HTTPS/HTTP)
Nginx->>WorkerService: Proxy /aiominionocodb to 9000 (if configured)
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Nitpick comments (3)
nix/docker/all_in_one/init/env-processor/env_processor.sh (1)
64-71: LGTM: Well-designed conditional logic for legacy compatibility.The enhanced condition properly respects existing database configurations by checking for:
- Legacy database file (
/var/noco.db)- Environment-based database configurations (
DATABASE_URL,NC_DB_JSON,NC_DB)This ensures PostgreSQL is only enabled by default when no existing database setup is detected, maintaining backward compatibility.
Minor typo: "compatiblity" should be "compatibility" in the comment.
nix/workflows/aiopusher/default.nix (2)
44-46: Fix parameter documentation typo.There's a typo in the parameter documentation - both
$2and$3are labeled as "arch tag 1".- # $2: arch tag 1 - # $3: arch tag 1 + # $2: arch tag 1 + # $3: arch tag 2
63-64: Fix parameter documentation.The parameter documentation has incorrect numbering - both parameters are labeled as
$1.- # $1?: string tag - # $1?: version tag + # $1?: string tag + # $2?: version tag
🛑 Comments failed to post (1)
nix/workflows/aiopusher/default.nix (1)
72-72:
⚠️ Potential issueCritical: Fix incorrect parameter ordering in function call.
The
image_pushfunction call has incorrect parameter ordering. Based on the function definition, the parameters should be: string tag, arch tag 1, arch tag 2, version tag (optional). The current call passes the version tag as the second parameter instead of the fourth.- image_push "''${1:-untagged}" "$2" x86_64-linux aarch64-linux + image_push "''${1:-untagged}" x86_64-linux aarch64-linux "$2"This ensures that:
$1: string tag for the manifest$2: first architecture tag (x86_64-linux)$3: second architecture tag (aarch64-linux)$4: optional version tag📝 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_push "''${1:-untagged}" x86_64-linux aarch64-linux "$2"🤖 Prompt for AI Agents
In nix/workflows/aiopusher/default.nix at line 72, the image_push function call has parameters in the wrong order. Rearrange the arguments so that the first parameter is the string tag (''${1:-untagged}''), the second is the first architecture tag (x86_64-linux), the third is the second architecture tag (aarch64-linux), and the optional fourth parameter is the version tag ($2). This corrects the parameter order to match the function definition.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nix/docker/all_in_one/init/default.nix (1)
71-71: Fix typo in comment.There's a spelling error in the comment.
- # backward compatiblity with legacy nocodb image + # backward compatibility with legacy nocodb image
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/docker/all_in_one/init/default.nix(1 hunks)nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)
🔇 Additional comments (2)
nix/docker/all_in_one/init/env-processor/env_processor.sh (2)
67-78: Approve the enhanced conditional default logic.The new conditional logic properly handles backward compatibility by:
- Only setting defaults when variables are unset
- Checking for legacy database file (
/var/noco.db)- Verifying no existing database configuration is present
- Similar logic for Redis/Valkey configuration
This prevents overriding user-defined values while maintaining backward compatibility.
10-13: Approve formatting and style improvements.The formatting changes improve code readability through:
- Consistent heredoc indentation
- Better spacing and line organization
- Cleaner continuation markers
These changes maintain functionality while enhancing maintainability.
Also applies to: 42-42, 52-52, 63-63, 86-86, 96-99, 106-113, 115-122, 124-128, 140-143, 146-149, 153-156
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nix/docker/all_in_one/init/default.nix (1)
71-73:⚠️ Potential issueThe critical
/vardirectory replacement issue remains unresolved.This is the same issue identified in the previous review. The symbolic link
ln -s /src/app/data /varstill replaces the entire/vardirectory, which can break system functionality by obliterating standard subdirectories like/var/log,/var/lib,/var/tmp, and/var/run.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nix/docker/all_in_one/init/default.nix (1)
82-82:⚠️ Potential issueCritical: The
/varsymlink replacement issue persists.This line still creates the problematic symlink that replaces the entire
/vardirectory, which was flagged in previous reviews as potentially breaking system functionality.As noted in previous reviews, replacing
/varentirely can break system functionality since it contains critical system directories like/var/log,/var/lib,/var/tmp, and/var/run.Please implement the suggested fix from the previous review:
- # backward compatiblity with legacy nocodb image - ln -s ${working_dir} /var + # backward compatibility with legacy nocodb image + mkdir -p /var/lib/nocodb + ln -s ${working_dir} /var/lib/nocodb/data
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nix/docker/all_in_one/default.nix(2 hunks)nix/docker/all_in_one/init/default.nix(3 hunks)nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/docker/all_in_one/default.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/docker/all_in_one/init/env-processor/env_processor.sh
🔇 Additional comments (4)
nix/docker/all_in_one/init/default.nix (4)
21-24: LGTM: Clean directory variable definitions.The new directory path variables provide clear separation of concerns and improve maintainability. The migration from
/var/libto/usr/app/dataas the working directory is well-structured.
39-72: LGTM: Well-structured migration framework.The migration functions provide a solid foundation for handling version upgrades and state management. The
kvstore_get/setintegration and state versioning approach follows good practices for container migrations.
74-79: Improved logging and more tolerant/rundirectory handling.The enhanced logging with the
log()function and more user-friendly warning message for missing/rundirectory improves the user experience.
122-126: LGTM: Proper sequencing of environment and service setup.Deferring environment processing and service compilation until after migration handling ensures that migration state can influence service configuration correctly.
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 (3)
nix/docker/all_in_one/confs/nginx.nix(2 hunks)nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)nix/docker/all_in_one/init/s6-services/configs/nocodb.conf(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/docker/all_in_one/init/s6-services/configs/nocodb.conf
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/docker/all_in_one/init/env-processor/env_processor.sh
🔇 Additional comments (1)
nix/docker/all_in_one/confs/nginx.nix (1)
155-155: Port change looks correct and consistent.The proxy_pass change from port 8080 to 8008 correctly aligns with the new NocoDB service port configuration. This change is consistent with the new HTTP server block which also proxies to port 8008.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/run (1)
5-5: Review the default DIRECTIVES value for s6-log.The default DIRECTIVES value
"n3 s2000000 T"sets specific logging parameters (3 log files, 2MB size limit, timestamp). Consider documenting these parameters or making them configurable.Add a comment explaining the DIRECTIVES parameters:
+# DIRECTIVES: n3 (keep 3 log files), s2000000 (2MB size limit), T (timestamp) importas -sCuD "n3 s2000000 T" DIRECTIVES DIRECTIVES
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)nix/docker/all_in_one/init/s6-services/configs/nocodb.main.conf(1 hunks)nix/docker/all_in_one/init/s6-services/configs/nocodb.worker.conf(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-log/consumer-for(0 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-log/pipeline-name(0 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/consumer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/pipeline-name(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/producer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-srv/producer-for(0 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/consumer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/notification-fd(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/pipeline-name(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/type(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/producer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/type(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb/type(1 hunks)
💤 Files with no reviewable changes (3)
- nix/docker/all_in_one/init/s6-services/services/nocodb-log/pipeline-name
- nix/docker/all_in_one/init/s6-services/services/nocodb-log/consumer-for
- nix/docker/all_in_one/init/s6-services/services/nocodb-srv/producer-for
✅ Files skipped from review due to trivial changes (11)
- nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/pipeline-name
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/type
- nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/producer-for
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/type
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/pipeline-name
- nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/consumer-for
- nix/docker/all_in_one/init/s6-services/services/nocodb/type
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/consumer-for
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/notification-fd
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/producer-for
- nix/docker/all_in_one/init/s6-services/configs/nocodb.worker.conf
🔇 Additional comments (15)
nix/docker/all_in_one/init/s6-services/configs/nocodb.main.conf (1)
3-3: LGTM! Port configuration updated for service separation.The port change from 8080 to 8008 correctly aligns the main service port with the broader restructuring to support separate main and worker services.
nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/run (1)
4-6: LGTM! Environment file modularization is well-structured.The updated environment file sourcing correctly implements the modular approach:
- Static main configuration from
nocodb.main.conf- Shared dynamic configuration from
nocodb.common.dynamic.env- Main service-specific configuration from
nocodb.main.dynamic.envThis separation enables proper configuration management for the main/worker service split.
nix/docker/all_in_one/init/s6-services/services/nocodb-main-log/run (2)
3-3: LGTM! Environment file updated for main service.The change from
nocodb.conftonocodb.main.confcorrectly aligns the logging service with the main service configuration.
8-14: LGTM! Log directory paths correctly separated for main service.The log directory paths have been appropriately updated from generic
nocodbtonocodb-main, ensuring clear separation between main and worker service logs. Both the conditional/var/log/nocodb-mainand fallback/run/log/nocodb-mainpaths are consistently updated.nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run (1)
8-19:✅ Verification successful
Verify shared database and directory usage between services.
Both main and worker services create the same database (
nocodb) and use the same working directory (/var/lib/nocodb). Please confirm this is intentional for data sharing between services.Run the following script to verify how the services are intended to interact:
🏁 Script executed:
#!/bin/bash # Description: Check for any configuration differences between main and worker services # Expected: Worker should have different configuration or clear documentation of shared resources echo "=== Checking configuration files ===" fd "nocodb.*\.conf$" nix/docker/all_in_one/init/s6-services/configs/ echo -e "\n=== Checking for worker-specific environment variables ===" rg -A 5 -B 5 "worker|WORKER" nix/docker/all_in_one/ echo -e "\n=== Checking for service separation documentation ===" rg -A 10 -B 10 "main.*worker|worker.*main" nix/docker/all_in_one/Length of output: 11760
Shared database and directory usage is intentional
I’ve confirmed that:
- There are two separate config files—
nix/docker/all_in_one/init/s6-services/configs/nocodb.main.confand…/nocodb.worker.conf—so each service has its own environment settings (e.g.NC_WORKER_CONTAINER=truein the worker).- Both
runscripts idempotently create/check the same Postgres database (nocodb) and the shared data directory (/var/lib/nocodb) to ensure the worker can operate on the same dataset as the main service.No changes required.
nix/docker/all_in_one/init/s6-services/services/nocodb-worker-log/run (2)
7-10: LGTM! Proper conditional logging setup.The conditional logic correctly checks if
/var/logis writable before attempting to create the logging directory there. The fallback to/run/log/nocodb-workerensures logging works regardless of filesystem permissions.
12-14: LGTM! Appropriate fallback logging configuration.The fallback logging setup ensures the service can always log to
/run/log/nocodb-workerwith proper ownership and permissions.nix/docker/all_in_one/init/env-processor/env_processor.sh (8)
6-8: LGTM! Clear separation of environment files.The split into common, main, and worker environment files provides good separation of concerns for the new service architecture.
12-15: LGTM! Improved dynamic password generation.The password generation logic is more robust with proper command substitution syntax.
38-43: LGTM! Proper environment variable routing.The logic correctly routes
aio_pass_main_*andaio_pass_worker_*prefixed variables to their respective environment files.
79-96: LGTM! Good backward compatibility logic.The conditional logic properly maintains backward compatibility with legacy nocodb images by checking for existing database configurations before enabling PostgreSQL by default.
113-118: LGTM! Clean dependency management abstraction.The
nocodb_dep_addfunction provides a clean abstraction for adding dependencies to both main and worker services.
148-149: LGTM! Improved Minio S3 endpoint configuration.The change from
http://localhost:9000tohttp://${aio_ssl_domain}makes the endpoint configurable and more flexible for different deployment scenarios.
188-197: LGTM! Proper worker service enablement logic.The worker enablement logic correctly:
- Creates the worker service content file
- Adds valkey dependency (required for worker coordination)
- Sets
NC_WORKER_CONTAINER=falsein the main environment- Provides appropriate logging
204-206: LGTM! Proper initialization of all environment files.The script correctly initializes all three environment files at startup.
nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nix/docker/all_in_one/init/default.nix (2)
56-56:⚠️ Potential issueCritical issue: Replacing /var directory with symlink breaks system functionality.
This symlink replaces the entire
/vardirectory (containing/var/log,/var/lib,/var/tmp,/var/run, etc.) with a link to/usr/app/data, which will break system and application functionality.As identified in previous reviews, this approach is fundamentally flawed. The
/vardirectory contains critical system subdirectories that should not be replaced. Consider creating specific subdirectory symlinks instead:-ln -s ${working_dir} /var +mkdir -p /var/lib/nocodb/data +ln -s ${working_dir} /var/lib/nocodb/data
86-92:⚠️ Potential issueMigration logic affected by problematic /var symlink.
The backward compatibility logic will be confused by the
/varsymlink created on line 56. Since/varpoints to/usr/app/data, the file checks may not work as intended for actual legacy migration scenarios.As noted in previous reviews, this logic needs to be updated once the
/varsymlink issue is resolved. The current paths assume the symlink exists, which creates circular and confusing references.
🧹 Nitpick comments (1)
nix/docker/all_in_one/confs/redis.nix (1)
4-4: Consider using standard Redis configuration filename.The configuration file is named
nocodb.confinstead of the conventionalredis.conf. While this works functionally, it may cause confusion during debugging or maintenance since administrators typically expect Redis configurations to follow standard naming conventions.Consider renaming to align with Redis conventions:
-writeTextDir "etc/nocodb.conf" '' +writeTextDir "etc/redis.conf" ''Note: This change would require updating the service script in
redis-srv/runto reference/etc/redis.confinstead of/etc/redis.conf.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
nix/docker/all_in_one/confs/redis.nix(1 hunks)nix/docker/all_in_one/default.nix(5 hunks)nix/docker/all_in_one/init/default.nix(3 hunks)nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)nix/docker/all_in_one/init/s6-services/services/redis-log/consumer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-log/notification-fd(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-log/pipeline-name(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-log/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-log/type(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-srv/producer-for(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-srv/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/redis-srv/type(1 hunks)nix/docker/all_in_one/init/s6-services/services/valkey-log/consumer-for(0 hunks)nix/docker/all_in_one/init/s6-services/services/valkey-log/pipeline-name(0 hunks)nix/docker/all_in_one/init/s6-services/services/valkey-srv/producer-for(0 hunks)nix/docker/all_in_one/init/s6-services/services/valkey-srv/run(0 hunks)
💤 Files with no reviewable changes (4)
- nix/docker/all_in_one/init/s6-services/services/valkey-srv/producer-for
- nix/docker/all_in_one/init/s6-services/services/valkey-log/pipeline-name
- nix/docker/all_in_one/init/s6-services/services/valkey-log/consumer-for
- nix/docker/all_in_one/init/s6-services/services/valkey-srv/run
✅ Files skipped from review due to trivial changes (7)
- nix/docker/all_in_one/init/s6-services/services/redis-log/notification-fd
- nix/docker/all_in_one/init/s6-services/services/redis-log/pipeline-name
- nix/docker/all_in_one/init/s6-services/services/redis-log/type
- nix/docker/all_in_one/init/s6-services/services/redis-srv/producer-for
- nix/docker/all_in_one/init/s6-services/services/redis-srv/type
- nix/docker/all_in_one/init/s6-services/services/redis-log/run
- nix/docker/all_in_one/init/s6-services/services/redis-log/consumer-for
🚧 Files skipped from review as they are similar to previous changes (2)
- nix/docker/all_in_one/default.nix
- nix/docker/all_in_one/init/env-processor/env_processor.sh
🔇 Additional comments (7)
nix/docker/all_in_one/confs/redis.nix (1)
10-10: Socket path change is consistent with Redis migration.The Unix socket path has been correctly updated from the valkey path to the Redis-specific path, maintaining consistency with the service migration.
nix/docker/all_in_one/init/s6-services/services/redis-srv/run (1)
1-8: Service setup script follows s6 best practices.The directory creation, ownership setting, and stderr redirection are implemented correctly for s6 supervision. The script ensures proper isolation and logging.
nix/docker/all_in_one/init/default.nix (5)
21-24: Path variable definitions look well-structured.The new path variables provide clear separation of concerns and improve code maintainability. The migration directory structure is logical for tracking upgrade states.
39-46: Logging and migration helper functions improve code organization.The
log()function provides consistent logging format, andmigrate_state_set()establishes a clean pattern for tracking migration states.
49-53: Improved error handling for missing /run directory.The change from exiting to logging a warning and creating the directory provides better resilience, allowing the container to continue functioning even in non-standard configurations.
66-66: User setup correctly migrated from valkey to redis.The change from
valkeytoredisuser is consistent with the service migration and follows the same security model.
94-99: Service compilation order properly handles dependencies.Moving environment processing and service compilation after migration setup ensures that legacy data is available when services start.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nix/docker/all_in_one/init/env-processor/env_processor.sh (1)
104-106: Default worker enable assignment is now correct
The previous issue of assigning_aio_ssl_enable_defaulthere has been resolved.
🧹 Nitpick comments (5)
nix/docker/all_in_one/init/env-processor/env_processor.sh (5)
12-15: Clarify random token generation
Theaio_pass_dynamicblock usestr+headpiped to/dev/urandomand a trailing:. It would help future maintainers to add a comment explaining the use of:(e.g., to ensure the substitution never fails) or simplify to a singleheadinvocation if possible.
38-43: Route AIO passthrough variables correctly
The newpass_mainandpass_workerpatterns ensure that AIO-prefixed variables are split into the right env files. Consider refactoring the twoechocalls into a helper to reduce duplication.
79-96: Improve consistency in file ex F438 istence checks
The tests mix/\"$var\"/NAMEquoting which can be simplified and made more readable. For example:- [ ! -f /"$kernal_env_store_dir"/DATABASE_URL ] && + [ ! -f "$kernal_env_store_dir/DATABASE_URL" ] &&Apply similar changes for all
/\"$kernal_env_store_dir\"/...checks and for$migrations_dir.
113-117: Centralize service dependency registration
The newnocodb_dep_addhelper is a solid abstraction. To harden it, consider verifying or creating$s6_services_temp_pathand itsdependencies.dsubdirectories before callingtouch.
167-176: Consolidate ACME env writes
Currently you append SSL email and then append domain in two separate here-docs. For clarity and atomicity, consider merging into a singlecat <<-EOFblock so that both values are always written together.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run
🔇 Additional comments (12)
nix/docker/all_in_one/init/env-processor/env_processor.sh (12)
6-8: Modularize dynamic env file paths
Splitting environment files into common, main, and worker scopes improves separation of concerns and makes it easier to manage service-specific variables.
18-18: Define migrations directory for legacy checks
Introducingmigrations_dircentralizes the path used for backward-compatibility checks. Approved.
21-22: Introduce Redis & Worker default flags
Setting_aio_redis_enable_default=trueand_aio_worker_enable_default=falsealigns with the new optional worker/Redis services. Good defaults.
25-25: Set default SSL domain
Defaulting_aio_ssl_domain_defaultto"localhost"is reasonable for local or dev setups.
53-53: Append non-AIO variables to common env
All other variables (excluding Docker defaults and AIO prefixes) are correctly captured in the common file.
73-74: Parse new AIO enable flags
The addition ofaio_redis_enableandaio_worker_enablein the key-dispatchcaseis correct.
122-123: Enable PostgreSQL dependency
Correctly invokesnocodb_dep_add postgresqlunder the Postgres enable flag.
132-133: Enable Minio dependency
The call tonocodb_dep_add miniocoupled with the S3 env writes is consistent.
147-148: Use dynamic SSL domain for S3 endpoint
Switching from hardcodedlocalhost:9000to${aio_ssl_domain}supports custom domains. Approved.
152-155: Populate Minio credentials securely
WritingMINIO_ROOT_USERandMINIO_ROOT_PASSWORDfrom the random token is correct. Ensure that$aio_pass_dynamicremains protected in logs.
178-186: Enable Redis service correctly
The new Redis block registers the dependency and setsNC_REDIS_URLover the Unix socket. This aligns with the migration from Valkey to Redis.
204-206: Initialize dynamic env files
Touching the three env files at startup guarantees they exist before writes. This is good practice.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
nix/docker/all_in_one/init/env-processor/env_processor.sh (2)
106-108: Critical bug: Wrong default value assignment.This is a known issue from previous reviews. Line 107 incorrectly assigns
_aio_ssl_enable_defaultinstead of_aio_worker_enable_defaulttoaio_worker_enable.
194-194: Verify worker service registration path.This is a known issue from previous reviews. The
touchcall writes to an incorrect path that will prevent the worker service from starting properly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
nix/docker/all_in_one/init/default.nix(3 hunks)nix/docker/all_in_one/init/env-processor/env_processor.sh(7 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run(1 hunks)nix/docker/all_in_one/init/s6-services/services/postgresql-db-create/type(1 hunks)nix/docker/all_in_one/init/s6-services/services/postgresql-db-create/up(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- nix/docker/all_in_one/init/s6-services/services/postgresql-db-create/type
🚧 Files skipped from review as they are similar to previous changes (2)
- nix/docker/all_in_one/init/s6-services/services/nocodb-worker-srv/run
- nix/docker/all_in_one/init/default.nix
🔇 Additional comments (10)
nix/docker/all_in_one/init/s6-services/services/postgresql-db-create/up (1)
3-10: LGTM: Proper s6 logging setup with privilege separation.The logging configuration correctly uses
s6-setuidgid s6logfor privilege separation and sets up the log directory with appropriate ownership. The pipeline structure and directives handling follow s6 best practices.nix/docker/all_in_one/init/s6-services/services/nocodb-main-srv/run (2)
3-6: LGTM: Well-structured environment variable sourcing.The script correctly sources environment variables from three distinct files (main, common, worker), which aligns with the modularized approach described in the AI summary. This supports the separation of concerns between different service contexts.
21-22: LGTM: Proper privilege dropping and service execution.The script correctly redirects stderr to stdout and uses
s6-setuidgidto drop privileges before executing the NocoDB service. The working directory change to/var/lib/nocodbis appropriate.nix/docker/all_in_one/init/env-processor/env_processor.sh (7)
6-8: LGTM: Clear environment file path declarations.The modularization into separate common, main, and worker environment files supports better separation of concerns and aligns with the service architecture changes.
20-27: LGTM: Well-documented default values with clear intent.The addition of
_aio_worker_enable_defaultand the comments about additional logic for postgres and redis defaults provide good documentation of the expected behavior.
40-46: LGTM: Enhanced environment passthrough with prefix-based routing.The new logic correctly routes environment variables with
aio_pass_main_andaio_pass_worker_prefixes to their respective environment files, supporting the modular service architecture.
81-98: LGTM: Intelligent conditional enablement with backward compatibility.The logic for conditionally enabling postgres and redis services based on migration markers and existing environment variables maintains backward compatibility while supporting the new architecture.
115-123: LGTM: Helper function for dependency management.The
nocodb_dep_addfunction provides a clean abstraction for adding dependencies to both main and worker services, reducing code duplication.
209-211: LGTM: Proper initialization of all environment files.The main execution correctly initializes all three environment files before processing, ensuring they exist for the subsequent functions.
153-154:✅ Verification successful
Verify S3 endpoint URL change.
The S3 endpoint URL was changed from
http://localhost:9000tohttp://${aio_ssl_domain}. Ensure this change is intentional and that the domain resolution works correctly in the container environment.Verify that the S3 endpoint change is correct and won't break existing deployments:
🏁 Script executed:
#!/bin/bash # Check if minio service configuration matches the new endpoint rg -A 5 -B 5 "9000.*minio" nix/docker/all_in_one/Length of output: 51
🏁 Script executed:
#!/bin/bash set -e # 1. Locate where aio_ssl_domain is defined in env_processor.sh echo ">>> Searching for aio_ssl_domain in env_processor.sh" rg -n "aio_ssl_domain" nix/docker/all_in_one/init/env-processor/env_processor.sh || true # 2. Find all occurrences of aio_ssl_domain in the all_in_one directory echo ">>> All references to aio_ssl_domain" rg -n "aio_ssl_domain" nix/docker/all_in_one || true # 3. Search for any minio service definitions echo ">>> References to 'minio'" rg -n "minio" nix/docker/all_in_one || true # 4. Search for any hard-coded 9000 port usage echo ">>> References to port 9000" rg -n "9000" nix/docker/all_in_one || trueLength of output: 7056
S3 endpoint change validated via NGINX proxy
We’ve confirmed that
NC_S3_ENDPOINT="http://${aio_ssl_domain}"(and its HTTPS variant) is intentional and works as designed:
- In env_processor.sh,
_aio_ssl_domain_default="localhost"(lines 23, 111), so by default the endpoint ishttp://localhost.- NocoDB will issue S3 calls against
http://<domain>/<bucket>(bucket isaiominionocodb), not directly on port 9000.- The NGINX config (
confs/nginx.nix) contains:
- location
/aiominionocodb→ proxy_passhttp://127.0.0.1:9000;- This routes all bucket-path requests to the MinIO service listening on port 9000.
- MinIO itself remains bound to port 9000 internally (
minio.conf), but external S3 traffic goes through NGINX on standard HTTP/HTTPS ports.No changes are required.
nix/docker/all_in_one/init/s6-services/services/postgresql-db-create/up
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
nix/docker/all_in_one/init/env-processor/env_processor.sh (1)
202-211:⚠️ Potential issueCritical fix: incorrect worker service registration path
The script touches the wrong path for the s6 worker service file on line 203. It should mirror the dependency structure used innocodb_dep_add.Apply this patch:
- touch "$s6_services_temp_path/nocodb/contents.d/nocodb-worker-srv" + touch "$s6_services_temp_path/nocodb-worker-srv/contents.d/nocodb-worker-srv"This ensures the worker service is properly registered and started under s6 supervision.
🧹 Nitpick comments (2)
nix/docker/all_in_one/init/s6-services/services/acme-srv/shell_up.sh (2)
15-24: Ensure ACME environment variables are defined before use
The domain and email variables (nc_aio_ssl_domain,nc_aio_ssl_email) are now correctly referenced inssl_renew(lines 19–20) andssl_run(lines 32–33), but they rely on sourcing/run/acme.dynamic.env. To avoid potential unbound variable issues (SC2154), add explicit checks right after loading the env file:. /run/acme.dynamic.env : "${nc_aio_ssl_domain:?nc_aio_ssl_domain must be set}" : "${nc_aio_ssl_email:?nc_aio_ssl_email must be set}"This will fail fast if the variables are missing.
Also applies to: 28-36
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 19-19: nc_aio_ssl_domain is referenced but not assigned.
(SC2154)
[warning] 20-20: nc_aio_ssl_email is referenced but not assigned.
(SC2154)
39-53: Add error handling when installing certificates
Inssl_install, the script copies certificates based on the domain (lines 45–47) and echoes the action (line 43). Consider verifying that the expected certificate files exist before copying to avoid partial or misleading installs:if [ ! -f "certificates/${nc_aio_ssl_domain}.crt" ] || [ ! -f "certificates/${nc_aio_ssl_domain}.key" ]; then echo "ERROR: Certificate or key not found for ${nc_aio_ssl_domain}" >&2 exit 1 fiThis guard will ensure the
cpcommands cannot silently fail.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/docker/all_in_one/init/env-processor/env_processor.sh(3 hunks)nix/docker/all_in_one/init/s6-services/services/acme-srv/shell_up.sh(3 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
nix/docker/all_in_one/init/s6-services/services/acme-srv/shell_up.sh
[warning] 19-19: nc_aio_ssl_domain is referenced but not assigned.
(SC2154)
[warning] 20-20: nc_aio_ssl_email is referenced but not assigned.
(SC2154)
🔇 Additional comments (15)
nix/docker/all_in_one/init/s6-services/services/acme-srv/shell_up.sh (1)
57-67: Certificate fetch logic is sound
The conditional check for an adhoc-managed certificate (line 57) and subsequent decision tossl_runvs.ssl_renew(lines 62–66) followed byssl_installis clear and aligns with the intended flow. The final&& ssl_installensures installation only on successful run/renew.nix/docker/all_in_one/init/env-processor/env_processor.sh (14)
4-9: New environment file paths and prefix are correctly introduced
The introduction ofnc_aio_env_prefix(line 4) and separate dynamic env files (nocodb_env_common_path,nocodb_env_main_path,nocodb_env_worker_pathon lines 6–8) properly sets up the split environment approach.
20-24: Default enablement flags are correctly defined
The_nc_aio_*_defaultvariables (lines 20–23) and default SSL domain (line 24) give sensible defaults for minio, SSL, and worker services.
39-54: env_passthrough correctly routes prefixed vars
The case branches on lines 40–50 now routenc_aio_pass_main_*, _worker_*, _minio_*, _acme_*into their respective files and skip othernc_aio_vars (line 53). This aligns with the PR objective to modularize env files.
69-87: env_aio_set mapping of prefixed keys is coherent
The added skip-cases fornc_aio_pass_*(lines 70–73) and the mapping ofnc_aio_ssl_domain,nc_aio_ssl_email,nc_aio_redis_enable, andnc_aio_worker_enable(lines 83–86) correctly capture all new env variables into shell variables.
110-118: Default assignment logic for remaining flags and domain is solid
Lines 110–118 assign defaults for minio, SSL, and worker enablement, and lines 120–122 set the default SSL domain. The omission of a default fornc_aio_ssl_emailis intentional, as an error is raised if SSL is enabled without an email.Also applies to: 120-122
125-132: nocodb_dep_add helper simplifies dependency creation
The new function (lines 125–132) creates matching dependency files for both main and worker services, reducing duplication.
134-143: PostgreSQL enablement flow is unchanged and correct
The block on lines 135–143 conditionally addspostgresqldependencies and writesDATABASE_URL. It preserves backward compatibility and meets the PR goal.
145-153: Minio enablement correctly writes credentials
Lines 145–153 addminioas a dependency and populate S3 bucket and credential variables using the generated dynamic pass key.
156-164: Conditional S3 endpoint protocol selection is sound
Selectinghttps://orhttp://based onnc_aio_ssl_enable(lines 156–164) is a clear implementation of SSL-aware endpoint configuration.
166-170: Minio service env file population is correct
The credentials are written tominio_env_path(lines 166–170) as intended.
174-187: SSL enablement block properly validates and adds dependencies
Lines 174–187 enforce thatnc_aio_ssl_emailis set if SSL is enabled, add theacmedependency, and write the email to the acme env file.
188-191: Unconditional domain write to acme env file
Writingnc_aio_ssl_domain(lines 188–191) even when SSL is disabled ensures the domain variable is always available for the ACME service.
192-200: Redis enablement logic is concise and correct
Addingredisdependencies andNC_REDIS_URL(lines 192–200) matches the updated architecture.
218-221: Touching all env files before processing is correct
Lines 218–221 create the common, main, and worker env files ahead of dumping the current environment, fitting the new modular file approach.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
nix/docker/all_in_one/init/env-processor/env_processor.sh (1)
209-209:⚠️ Duplicate: Worker service path mismatch
Thetouchpath here still points to…/nocodb/contents.d/nocodb-worker-srvbut should be under…/nocodb-worker-srv/contents.d/nocodb-worker-srv.
This was flagged in a previous review.
🧹 Nitpick comments (5)
nix/docker/all_in_one/init/env-processor/env_processor.sh (5)
12-15: Simplify dynamic password generation
The current block withtr … | head -c 16plus a trailing:works, but you could simplify this to a single pipeline. For example:-nc_aio_pass_dynamic="$({ - tr -dc A-Za-z </dev/urandom | head -c 16 - : -})" +nc_aio_pass_dynamic="$(tr -dc A-Za-z </dev/urandom | head -c 16)"This removes the no-op
:and the subshell block, improving readability.
91-100: Optional: Simplify file-existence checks for Postgres default
You have a multi-lineif [ ! -f … ] && [ ! -f … ]…block. For readability, consider looping over the filenames or quoting the entire path in one statement:-for f in from_legacy_image_with_sqlite DATABASE_URL NC_DB_JSON NC_DB; do - if [ -f "$migrations_dir/$f" ] || [ -f "$kernal_env_store_dir/$f" ]; then - nc_aio_postgres_enable=false - break - fi -done -nc_aio_postgres_enable=${nc_aio_postgres_enable:-true} +files=(from_legacy_image_with_sqlite DATABASE_URL NC_DB_JSON NC_DB) +for f in "${files[@]}"; do + if [ -f "$migrations_dir/$f" ] || [ -f "$kernal_env_store_dir/$f" ]; then + nc_aio_postgres_enable=false + break + fi +done +nc_aio_postgres_enable=${nc_aio_postgres_enable:-true}This reduces nesting and makes it easier to add/remove file checks in the future.
102-107: Optional: Normalize Redis-URL existence check
Similarly, changeif [ ! -f /"$kernal_env_store_dir"/NC_REDIS_URL ]; thento
if [ ! -f "$kernal_env_store_dir/NC_REDIS_URL" ]; thento avoid leading
//in the path and improve clarity.
174-187: SSL activation flow and email validation
You correctly bail out ifnc_aio_ssl_emailis not set. As a further nit, you may want to reject empty‐string emails ([ -z "$nc_aio_ssl_email" ]) to prevent silent misconfiguration.
223-225: Ensure parent directories exist before touching
You touch the three env files at startup, but if/runor subdirs aren’t present the script will fail. Consider adding:mkdir -p "$(dirname "$nocodb_env_common_path")" mkdir -p "$(dirname "$nocodb_env_main_path")" mkdir -p "$(dirname "$nocodb_env_worker_path")"before the
touchcalls for resilience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/docker/all_in_one/init/env-processor/env_processor.sh(3 hunks)
🔇 Additional comments (11)
nix/docker/all_in_one/init/env-processor/env_processor.sh (11)
4-4: Environment prefix and file paths are well-defined
Thenc_aio_env_prefixand the three separatenocodb_env_*_pathvariables are clear and correctly scoped. This sets up the modular env-file structure as intended.Also applies to: 6-8
18-24: Defaults for migrations and service-enable flags look good
Themigrations_dirand the_nc_aio_*_defaultvariables are correctly initialized. These defaults will ensure backward compatibility and predictable behavior when flags are not set.
80-87: Mapping ofnc_aio_*vars to shell variables is correct
Each key inkernel_env_store_diris being properly assigned to a local shell variable. The catch-all*) log ignoring unknown…will help surface unexpected keys.
110-118: Default assignments for MinIO, SSL, Worker, and SSL domain are appropriate
The fallbacks using"$_nc_aio_*_default"ensure that unset flags get sensible defaults. The ordering preserves backward compatibility.Also applies to: 120-122
125-132: Service dependency helper is clean and DRY
The newnocodb_dep_add()function correctly creates dependencies for bothmainandworkerservices. This removes duplication from theenv_aio_actsection.
135-143: Postgres activation and env injection looks solid
Usingif "$nc_aio_postgres_enable"; then(leveragingtrue/falsebuilt-ins) is concise. TheDATABASE_URLis appended correctly.
145-164: MinIO setup and S3 endpoint logic is correct
The script handles both HTTP and HTTPS endpoints based on the SSL flag, and writes the correct set ofNC_S3_*vars. Nice separation betweencommonandminioenv files.
166-170: MinIO container credentials injection is accurate
The root user and password get the same dynamic secret, as intended.
188-190: Always include SSL domain in ACME env
Appendingnc_aio_ssl_domaineven when SSL is disabled ensures the ACME container can reference the domain later. This is a sensible fallback.
192-197: Redis activation and URL injection is correct
The newredisservice gets its socket URL correctly appended to the common env file, and the dependency helper is invoked.
211-216: NC_WORKER_CONTAINER flag injection is correct
SettingNC_WORKER_CONTAINER=falsein the main env file follows the intended design to separate main and worker containers.
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)
nix/docker/all_in_one/init/s6-services/services/postgresql-srv/run(2 hunks)
🔇 Additional comments (1)
nix/docker/all_in_one/init/s6-services/services/postgresql-srv/run (1)
12-12: LGTM: Ownership extension aligns with PostgreSQL requirements.Adding
/var/lib/postgresqlto the ownership adjustment ensures that PostgreSQL has proper permissions on both runtime and data directories, which is essential for the service to function correctly.
nix/docker/all_in_one/init/s6-services/services/postgresql-srv/run
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh (1)
26-26: Add logging around self-signed cert generationThe concise
[ -d "$nc_aio_ssl_domain" ] || gen_selfsignedis fine but emits no logs. Consider a more verbose pattern for better observability:- [ -d "$nc_aio_ssl_domain" ] || gen_selfsigned + if [ -d "$nc_aio_ssl_domain" ]; then + echo "Self-signed cert for $nc_aio_ssl_domain already exists." + else + echo "Generating self-signed certificate for $nc_aio_ssl_domain" + gen_selfsigned + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh
[warning] 17-17: nc_aio_ssl_domain is referenced but not assigned.
(SC2154)
🔇 Additional comments F438 (1)
nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh (1)
17-17:❌ Incorrect review comment
Ensure the new environment variable is set before use
The updated
--domains "$nc_aio_ssl_domain"flag assumes thatnc_aio_ssl_domainis exported by/run/acme.dynamic.env. To avoid an unhelpful “referenced but not assigned” failure at runtime, add a guard at the top of the script:#!/bin/sh -e + : "${nc_aio_ssl_domain:?Environment variable nc_aio_ssl_domain must be set}"This will cause an immediate, clear error if the variable is missing.
Run this to verify it’s actually defined in your env processor:
🏁 Script executed:
#!/bin/bash # Verify nc_aio_ssl_domain is declared rg 'nc_aio_ssl_domain=' nix/docker/all_in_one/init/env-processor/env_processor.shLength of output: 223
I’d like to double-check how the dynamic env is sourced and whether
nc_aio_ssl_domainis always written and exported. Let’s grab the top ofshell_up.shand inspect how/run/acme.dynamic.envis generated in the env processor:
🏁 Script executed:
#!/bin/bash # Show the beginning of the service script to confirm env sourcing echo "=== shell_up.sh (top) ===" sed -n '1,200p' nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh # Find where the dynamic env file is written in the processor echo echo "=== env_processor.sh (dynamic env writes around) ===" rg -n 'dynamic\.env' nix/docker/all_in_one/init/env-processor/env_processor.sh -C5 # Check for any exports of nc_aio_ssl_domain echo echo "=== env_processor.sh (export statements) ===" rg -n '^ *export ' nix/docker/all_in_one/init/env-processor/env_processor.sh -C2Length of output: 1631
I’d like to confirm how and where
acme.dynamic.envis actually written by the env processor to ensurenc_aio_ssl_domainends up in that file. Let’s dump more ofenv_processor.shand look for write/redirection logic:
🏁 Script executed:
#!/bin/bash # Show total lines and first chunk of the processor echo "=== env_processor.sh line count ===" wc -l nix/docker/all_in_one/init/env-processor/env_processor.sh echo echo "=== env_processor.sh (1–200) ===" sed -n '1,200p' nix/docker/all_in_one/init/env-processor/env_processor.sh echo echo "=== Look for redirections involving dynamic env paths ===" rg -n '>' nix/docker/all_in_one/init/env-processor/env_processor.sh | grep -E 'env_path' echo echo "=== Look for echo/printf writing env vars ===" rg -n -C2 'echo .*=' nix/docker/all_in_one/init/env-processor/env_processor.sh rg -n -C2 'printf' nix/docker/all_in_one/init/env-processor/env_processor.shLength of output: 7078
Let’s inspect the tail of the env processor to see how and when
/run/acme.dynamic.envis initialized and those functions are invoked:
🏁 Script executed:
#!/bin/bash echo "=== env_processor.sh (200–237) ===" sed -n '200,237p' nix/docker/all_in_one/init/env-processor/env_processor.shLength of output: 965
No guard needed—
nc_aio_ssl_domainis always setThe env‐processor writes a
nc_aio_ssl_domain="…"line (falling back to the defaultlocalhost) into/run/acme.dynamic.envevery time before it’s sourced byshell_up.sh. That means by the point you hitminica … --domains "$nc_aio_ssl_domain"the variable will always be set, so you can safely ignore the suggested
: "${nc_aio_ssl_domain:?…}"guard.Likely an incorrect or invalid review comment.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 17-17: nc_aio_ssl_domain is referenced but not assigned.
(SC2154)
nix/docker/all_in_one/init/s6-services/services/acme-selfsigned/shell_up.sh
Show resolved
Hide resolved
a0a77f2 to
cacab6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
nix/docker/all_in_one/init/s6-linux-init/default.nix (1)
33-34: Fix typo in comment.There appears to be a typo in the comment. "upstall" should likely be "install".
- # LOCALE_ARCHIVE is required by upstall postgres migrated db + # LOCALE_ARCHIVE is required by install postgres migrated dbAlternatively, if this refers to an "upgrade/install" operation, consider:
- # LOCALE_ARCHIVE is required by upstall postgres migrated db + # LOCALE_ARCHIVE is required by upgrade/install postgres migrated db
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nix/docker/all_in_one/default.nix(5 hunks)nix/docker/all_in_one/init/s6-linux-init/default.nix(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- nix/docker/all_in_one/default.nix
🔇 Additional comments (1)
nix/docker/all_in_one/init/s6-linux-init/default.nix (1)
38-38: LGTM: Locale archive environment variable addition.The addition of
LOCALE_ARCHIVE=/lib/locale/locale-archiveis correctly formatted and necessary for PostgreSQL operations that require locale data. This aligns with the broader changes upgrading PostgreSQL to version 16 and adding glibc locale support.
Change Summary
replace nocodb/nocodb with nocodb/nocodb_aio
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
dependencies