Conversation
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@install/dependencies/build_env.sh`:
- Around line 168-175: The script branches to call get_storage_keys when the
user chooses S3 but never sets the STORAGE variable, causing failures under set
-u; update the code so STORAGE is set to a valid value when S3 is chosen (e.g.,
set STORAGE="s3")—either inside get_storage_keys or immediately after calling
get_storage_keys in the case for [Yy]*—and ensure any required export or default
fallback remains for other branches; reference the STORAGE variable and the
get_storage_keys call to locate where to add the assignment.
- Around line 263-271: The heredoc writing to BUILD_ENV_PATH currently includes
leading spaces before each AWS_* line (inside the STORAGE conditional), which
will be emitted literally into the .env; update the heredoc in build_env.sh so
the variable lines (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_BUCKET,
AWS_REGION, AWS_ENDPOINT) have no leading whitespace (or switch to an
unindented/left-aligned EOF marker) so the resulting file contains lines like
AWS_ACCESS_KEY_ID="..." with no leading spaces.
- Around line 6-11: The BUILD_ENV_PATH assignment uses an unsafe expansion of
REPOSITORY_PATH which under set -u will cause a crash before the subsequent
validation runs; fix by validating REPOSITORY_PATH exists before using it (move
the if [ -z "${REPOSITORY_PATH:-}" ] check above the BUILD_ENV_PATH assignment)
or change the assignment to use safe expansion (e.g.,
BUILD_ENV_PATH="${REPOSITORY_PATH:-}/.env") so the script does not error out
when REPOSITORY_PATH is unset.
In `@install/scripts/entrypoint.sh`:
- Around line 27-34: The script prints "✅ Migrations are all up" unconditionally
even when SKIP_MIGRATIONS is set; update the logic so the success message is
only echoed after running bundle exec rails db:migrate (when SKIP_MIGRATIONS is
unset), and emit a clear different message when SKIP_MIGRATIONS is set (or
remove the unconditional echo). Locate the SKIP_MIGRATIONS conditional and move
or conditionally guard the echo "✅ Migrations are all up" so it only runs after
the bundle exec rails db:migrate branch, leaving the existing echo "⚠️ Skipping
migrations!" in the else branch.
♻️ Duplicate comments (4)
install/scripts/entrypoint.sh (1)
1-14: Add fail-fast behavior for robustness.The script lacks
set -e, so failures inbundle install,rake, orrails db:migratewon't halt execution, potentially starting the container in a broken state. This was flagged in a previous review.install/dependencies/generate_vapid_keys.sh (1)
21-26: Add validation for extracted VAPID keys.If
grepreturns no matches (e.g., the Rails task output format changes or fails silently), empty keys are exported and the installer proceeds with invalid configuration. This will cause push notification failures at runtime.Proposed fix
VAPID_PUBLIC_KEY=$(echo "$output" | grep 'VAPID_PUBLIC_KEY' | cut -d'=' -f2) VAPID_PRIVATE_KEY=$(echo "$output" | grep 'VAPID_PRIVATE_KEY' | cut -d'=' -f2) +if [ -z "$VAPID_PUBLIC_KEY" ] || [ -z "$VAPID_PRIVATE_KEY" ]; then + echo "❌ Failed to extract VAPID keys from generator output" + exit 1 +fi + # Export the keys for use by calling script export VAPID_PUBLIC_KEY export VAPID_PRIVATE_KEYinstall/dependencies/build_env.sh (2)
114-125: InitializeCOMPOSE_PROFILESbefore the case statement to prevent crash.
COMPOSE_PROFILESis only set inbuild_local_database(line 68). When the user chooses external database, line 259 expands an unset variable underset -u, crashing the script.Proposed fix
+COMPOSE_PROFILES="" + case $yn in [Yy]*) EXTERNAL_DATABASE=true build_external_database ;; [Nn]*) EXTERNAL_DATABASE=false build_local_database ;; esac
191-192: Fix typo in variable name:MAPS_API_PROIVDER→MAPS_API_PROVIDER.The misspelled variable reference means any pre-existing
MAPS_API_PROVIDERvalue is ignored, always defaulting to "here".Proposed fix
read -r -p "HERE API KEY: " MAPS_API_KEY </dev/tty -MAPS_API_PROVIDER=${MAPS_API_PROIVDER=-here} +MAPS_API_PROVIDER=${MAPS_API_PROVIDER:-here}
🧹 Nitpick comments (1)
install/dependencies/build_env.sh (1)
104-109: Consider hiding password input withread -rs.Database password is visible as the user types. The same applies to SMTP_PASSWORD (line 136) and AWS_SECRET_ACCESS_KEY (line 158). Using
-ssuppresses echo for sensitive fields.Example fix for this block
while [ -z "${DATABASE_PASSWORD:-}" ]; do - read -r -p "Database password: " DATABASE_PASSWORD </dev/tty + read -rs -p "Database password: " DATABASE_PASSWORD </dev/tty + echo # newline after hidden input if [ -z "$DATABASE_PASSWORD" ]; then echo "❌ Database password cannot be empty" fi done
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@install/install.sh`:
- Around line 39-42: The script currently sets REPOSITORY_PATH using only
DECIDIM_PATH, ignoring a user-set REPOSITORY_PATH; update the assignment for the
REPOSITORY_PATH variable so it first uses the environment-provided
REPOSITORY_PATH if present, then falls back to DECIDIM_PATH, and finally to the
default "/opt/decidim". Modify the REPOSITORY_PATH assignment (the
REPOSITORY_PATH variable near the top of the script) to evaluate in that
priority order and keep REPOSITORY_URL and REPOSITORY_BRANCH unchanged.
In `@install/up.sh`:
- Around line 6-12: The script assumes REPOSITORY_PATH is set and fails under
set -u; before defining ENV_FILE add a safe default for REPOSITORY_PATH (e.g.,
use shell parameter expansion to fall back to a sensible path such as the
repository root or the script's parent directory) so
ENV_FILE="${REPOSITORY_PATH}/.env" works when up.sh is run standalone; update
install/up.sh to set REPOSITORY_PATH if unset (affecting the ENV_FILE assignment
and subsequent checks) using the parameter expansion pattern so no runtime error
occurs when REPOSITORY_PATH is missing.
♻️ Duplicate comments (3)
install/dependencies/build_env.sh (1)
194-195: Fix map provider default assignment (-heretypo).
Line 195 setsMAPS_API_PROVIDERto-herewhen unset, which is likely invalid and inconsistent with Line 252’s default. Use the standard:-heredefault.🛠️ Proposed fix
-MAPS_API_PROVIDER=${MAPS_API_PROVIDER=-here} +MAPS_API_PROVIDER=${MAPS_API_PROVIDER:-here}If you want to confirm how the variable is consumed across scripts and compose files:
#!/bin/bash rg -n "MAPS_(API_)?PROVIDER" -g '*.sh' -g '*.yml' -g '*.yaml'install/install.sh (1)
67-75: Add integrity verification for deploy.zip before unzip.
Lines 67‑75 fetch and extract a remote archive that is later sourced. Without checksum/signature verification, a compromised release becomes RCE.🔒 Suggested checksum verification
curl -fsSL \ --retry 3 \ --retry-delay 2 \ --connect-timeout 30 \ --max-time 300 \ --progress-bar \ -o "$TMP/deploy.zip" \ "$REPOSITORY_URL/releases/download/latest/deploy.zip" + +curl -fsSL \ + -o "$TMP/deploy.zip.sha256" \ + "$REPOSITORY_URL/releases/download/latest/deploy.zip.sha256" +(cd "$TMP" && sha256sum -c deploy.zip.sha256)If you want to confirm whether the release workflow already publishes checksums:
#!/bin/bash rg -n "sha256|checksum" .github/workflowsREADME.md (1)
169-169: Add period after “etc.”
Line 169 should use “etc.).” for correct punctuation.✏️ Suggested edit
-| **MAPS_PROVIDER** | `here` | app | Selects map provider (here, mapbox, google, etc). | +| **MAPS_PROVIDER** | `here` | app | Selects map provider (here, mapbox, google, etc.). |
🧹 Nitpick comments (1)
install/dependencies/build_env.sh (1)
137-141: Hide secret inputs in prompts.
Lines 138 and 160 echo secrets to the terminal. Usingread -sprevents shoulder‑surfing and keeps sensitive values out of scrollback.🔒 Proposed update
-read -r -p "SMTP_PASSWORD: " SMTP_PASSWORD </dev/tty +read -r -s -p "SMTP_PASSWORD: " SMTP_PASSWORD </dev/tty +echo @@ -read -r -p "Secret Access Key: " AWS_SECRET_ACCESS_KEY </dev/tty +read -r -s -p "Secret Access Key: " AWS_SECRET_ACCESS_KEY </dev/tty +echoPlease verify the prompt flow still renders clearly in your target terminals after masking input.
Also applies to: 159-161
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@install/docker-compose.yml`:
- Line 87: The Traefik dashboard is exposed without authentication via the
command flag --entrypoints.traefik.address=:8080; either disable the dashboard
by removing/enforcing --api=false in the Traefik service command or add
authentication middleware and attach it to the traefik router: create a basic
auth middleware (htpasswd) and reference it from the router that serves the
dashboard (the router named/labelled "traefik"), or add the auth via a
static/dynamic provider referenced by the service; update the docker-compose
Traefik service command/labels and the router configuration to ensure the
dashboard endpoint is protected.
- Around line 63-69: The worker service's environment omits a value for
BUNDLE_GEMFILE while the app service sets BUNDLE_GEMFILE=Gemfile.wrapper,
causing inconsistent Gemfile usage; update the worker service environment to
explicitly set BUNDLE_GEMFILE=Gemfile.wrapper so both services use the same
Gemfile (look for the worker environment block and the app service
BUNDLE_GEMFILE setting to ensure they match).
- Around line 19-25: The traefik label traefik.http.routers.app.rule currently
uses Host(`$DECIDIM_DOMAIN`) which Docker Compose will interpolate at compose
time and produce malformed labels; update the label to escape the dollar sign so
the runtime container receives the literal variable (use $$DECIDIM_DOMAIN) so
Traefik can resolve it at runtime. Locate the labels block containing
traefik.http.routers.app.rule and replace the single-dollar variable with a
double-dollar escape to prevent compose interpolation.
In `@install/README.md`:
- Line 249: The README has duplicate and incorrect section numbers: the heading
"6.3 Email Domain Authentication" should be renumbered to follow the prior
subsection (e.g., change the second "6.3" to "6.4" or the appropriate next
number), and the "Step 7" subsection references that currently use 6.1, 6.2, 6.3
must be updated to 7.1, 7.2, 7.3 respectively; locate the headings and the "Step
7" block in install/README.md (search for the literal headings "6.3 Email Domain
Authentication" and "Step 7") and update the numeric prefixes so numbering is
sequential and consistent across the file.
- Around line 294-297: The restart command in the README targets a non-existent
Docker service name ("sidekiq"); update the command to use the actual service
name defined in docker-compose.yml (replace "sidekiq" with "worker"), or
alternatively rename the service in docker-compose.yml to "sidekiq" if that
naming is preferred—ensure the README's "Restart Sidekiq if needed" section and
the docker-compose service names (worker) match.
- Around line 327-332: The README's update steps reference running "git pull"
but the installer delivers a zip (no git repo); remove or replace the git pull
step in the sequence shown and instead document the correct update flow: show
running "docker compose pull" then "docker compose up -d" to refresh images, and
add a short note telling users that to update installer scripts or configuration
they must re-run the installer or download the latest release (link to the
releases page); update the block in install/README.md that contains the commands
and add the clarifying note suggested in the review.
- Around line 421-430: The log command example uses the container name "decidim"
but docker compose logs expects a service name; update the example line "docker
compose logs -f decidim" to use the service name "app" instead (leave the "db"
example as-is), and add a brief note or comment clarifying that docker compose
logs accepts service names while docker logs uses container names so readers use
"app" for compose commands.
- Around line 69-73: The displayed ssh-keygen command is truncated ("ssh-keygen
-t rsa -b 4096 -C \"") causing a syntax error; update the snippet so the command
includes a placeholder email and the closing quote (e.g., replace the truncated
string with ssh-keygen -t rsa -b 4096 -C "your_email@example.com") and ensure
the fenced code block is properly closed; target the README snippet containing
the ssh-keygen command.
- Around line 7-14: The table of contents links are broken because the actual
headings differ: the heading for server creation contains the parenthetical
"(Hetzner Example)" so its auto-generated anchor is
"#step-1-create-a-server-hetzner-example" (not "#step-1-create-a-server"), and
the final setup heading is numbered "Step 7: Complete Setup" while the TOC
points to "#step-6-complete-setup"; update the TOC entry 2 to use
"#step-1-create-a-server-hetzner-example" (or remove the parenthetical from the
heading) and update TOC entry 7 (the "Step 6/7" entry) so the link matches the
heading by either renaming the heading to "Step 6: Complete Setup" or changing
the TOC link to "#step-7-complete-setup".
🧹 Nitpick comments (5)
install/docker-compose.yml (4)
76-77: Replace deprecatedlinkswithdepends_on.The
linksdirective is deprecated in modern Docker Compose. Service discovery via container names works automatically on the default network. Usedepends_onif you need startup ordering.♻️ Proposed fix
- links: - - cache + depends_on: + - cache
84-84: Consider reducing Traefik log level for production.
DEBUGlevel logging is verbose and may expose sensitive request/response details. For a production deployment guide,INFOorWARNwould be more appropriate defaults.♻️ Proposed fix
- - --log.level=DEBUG + - --log.level=INFO
46-52: Inconsistentpull_policybetween app and worker services.The worker service has
pull_policy: always(line 51) but the app service doesn't. For consistency and to ensure both services use the same image version, consider adding the same policy to the app service.♻️ Proposed fix for app service
app: container_name: decidim image: ${DECIDIM_IMAGE:-decidim/decidim:latest} + pull_policy: always command: ["bin/rails", "server", "-b", "0.0.0.0"]
116-121: Consider adding health checks for Redis cache service.The PR objectives mention adding container health checks as a planned improvement. Adding a health check to Redis would allow dependent services to wait for Redis to be ready.
♻️ Example health check
cache: image: redis:8-alpine container_name: decidim_cache volumes: - cache_data:/data restart: always + healthcheck: + test: ["CMD", "redis-cli", "ping"] + interval: 10s + timeout: 5s + retries: 3install/README.md (1)
182-188: Use proper headings instead of bold emphasis for option categories.Lines 182 and 185 use bold text (
**Free Options**,**Paid Options**) as pseudo-headings. This affects document structure and accessibility. Consider using proper heading levels.📝 Proposed fix
-**Free Options** +#### Free Options - [Gmail SMTP](https://support.google.com/a/answer/176600) (500 emails/day limit) -**Paid Options** +#### Paid Options - [Scaleway](https://www.scaleway.com/en/transactional-email-tem/)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@install/README.md`:
- Around line 7-15: Update the table of contents numbering so the final entry
uses "9" instead of duplicating "8"; specifically edit the TOC line for
"Troubleshooting" (the entry labeled "Troubleshooting") to change its leading
number from 8 to 9 so the sequence reads 1–9 and the entries "Step 7: Complete
Setup" and "Troubleshooting" are correctly numbered.
- Line 360: The troubleshooting instruction "Run `./install.sh` again"
contradicts the documented installation method using the curl|bash pipeline;
update the sentence in install/README.md to instruct users to re-run the
documented installer command (the curl -sSL … | bash pipeline used earlier) or
add a note explaining how to re-download the install script before running
`./install.sh`; ensure you update the exact troubleshooting line so it
references the curl|bash command or the download step rather than assuming
`./install.sh` already exists.
- Line 142: Update the README's installer command so it points to a valid
installer script instead of the broken https://decidim.org/install endpoint:
replace the curl invocation line (the one containing "curl -fsSL
https://decidim.org/install | bash") with a working URL to the raw installer
(for example a GitHub raw URL to install/install.sh) or instruct users to run
the local script (e.g., "bash install/install.sh" or "curl -fsSL
<raw-github-url> | bash"); ensure the new command is the one shown in the README
so users can successfully fetch and run the installer.
♻️ Duplicate comments (4)
install/README.md (4)
296-297: Incorrect service name in restart command.The command references
sidekiqbut the docker-compose.yml defines the service asworker. This command will fail with "no such service" error.🐛 Proposed fix
# Restart Sidekiq if needed -docker compose restart sidekiq +docker compose restart worker
250-250: Fix duplicate section numbering.Line 250 shows "### 6.3 Email Domain Authentication" but line 240 already uses "### 6.3 Security Best Practices". The email authentication section should be numbered 6.4.
📝 Proposed fix
-### 6.3 Email Domain Authentication +### 6.4 Email Domain Authentication
422-424: Incorrect service name in logs command.The command uses
decidimwhich is the container name, butdocker compose logsexpects the service nameapp. While container names work withdocker logs, the command as shown will fail with docker compose.🐛 Proposed fix
# Application logs -docker compose logs -f decidim +docker compose logs -f app # Database logs docker compose logs -f db
328-333: Update instructions incompatible with zip-based installation.The installation guide uses
curl | bashwhich downloads a zip file, not a git repository. Users won't have a git repo to rungit pullon. This command will fail.♻️ Proposed fix
### Updating Decidim ```bash cd /opt/decidim -git pull +# Pull the latest Docker images docker compose pull +# Restart with updated images docker compose up -d
+> Note: To update the installer scripts or configuration files, re-run the installer or download the latest release from the releases page.
</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>install/README.md (1)</summary><blockquote> `183-186`: **Use proper headings instead of emphasis for section labels.** The "Free Options" and "Paid Options" labels use bold emphasis (`**text**`) instead of proper Markdown headings. Using headings improves document structure and accessibility. <details> <summary>♻️ Proposed refactor</summary> ```diff ### 5.1 Choose an Email Provider -**Free Options** +#### Free Options + - [Gmail SMTP](https://support.google.com/a/answer/176600) (500 emails/day limit) -**Paid Options** +#### Paid Options + - [Scaleway](https://www.scaleway.com/en/transactional-email-tem/) - [Mailgun](https://mailgun.com)
greenwoodt
left a comment
greenwoodt
left a comment
There was a problem hiding this comment.
Very very solid README in line with the work on the script @davidbeig congrats matey! 🥇
Checkout my comments on some small suggestions.
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
microstudi
left a comment
microstudi
left a comment
There was a problem hiding this comment.
Please check codderabbit suggestions, specially those I've commented.
Also, apply @greenwoodt spelling/corrections and close all comments fixed.
In general, we should try to keep the custom ".env" file as small as possible, so if we can directly define a var that it is unlikely to be change let's keep it in the docker-compose, for instance:
BUNDLE_GEMFILE={$BUNDLE_GEMFILE:-Gemfile.wrapper}
We should also keep consistency between the container_name and the service name in the docker-compose.yml. For instance, decidim service is called app but the container name is decidim, however the worker is called worker and the container decidim_worker.
Maybe we could just skip the container_name directive (so we make this compatible with the rollout plugin for instance). If not, let's keep consistency (decidim_app).
I'll review again when done.
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
Co-authored-by: Tom Greenwood <101816158+greenwoodt@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install/dependencies/build_env.sh`:
- Around line 137-138: The SMTP password is being entered visibly; change the
interactive read for SMTP_PASSWORD to use silent mode (read -s) while keeping
the prompt on /dev/tty, and after reading the password print a newline so the
prompt cursor moves to the next line; keep SMTP_USERNAME as a normal visible
read. Update the read invocation that references SMTP_PASSWORD to use the -s
flag and ensure it still reads from /dev/tty and echoes a newline afterward.
- Around line 106-111: The password prompt loop currently reads
DATABASE_PASSWORD with visible input; update the read invocation used in the
while loop to suppress echo by using the -s flag (i.e., `read -s -r -p ...`
while still reading from /dev/tty) and after the read emit a newline (e.g., a
simple printf or echo) so the prompt formatting remains correct; locate the loop
that references DATABASE_PASSWORD and the existing read command to apply this
change.
- Around line 116-125: The case on variable yn lacks a default branch so
unexpected input leaves DATABASE_* unset and later breaks under set -u; update
the case handling around yn to add a default *) arm (after the [Yy]* and [Nn]*
arms) that either re-prompts the user or prints an error and exits non-zero, and
ensure EXTERNAL_DATABASE, build_external_database, or build_local_database are
not bypassed; reference the yn variable and the case block, and the
build_external_database / build_local_database actions when making the change.
- Around line 39-47: The interactive prompts for DECIDIM_APPLICATION_NAME and
DECIDIM_DOMAIN currently accept empty input; add validation loops like the DB
prompts to require non-empty values and re-prompt until a non-blank string is
provided. Specifically, update the read prompts that set
DECIDIM_APPLICATION_NAME and DECIDIM_DOMAIN in build_env.sh to check the
variable after read and repeat the prompt (printing an error message) if the
value is empty so downstream values (e.g., CERTIFICATE_EMAIL) are never built
from blank inputs. Ensure you preserve the existing messages and use the same
read -r -p ... </dev/tty pattern.
- Around line 194-195: The script uses MAPS_API_PROVIDER but docker-compose
expects MAPS_PROVIDER, so replace the MAPS_API_PROVIDER variable with
MAPS_PROVIDER wherever it’s set/used (e.g., change the default assignment
currently written as MAPS_API_PROVIDER=${MAPS_API_PROVIDER=-here} to use
MAPS_PROVIDER and the later default expansion
MAPS_PROVIDER="${MAPS_PROVIDER:-here}"); ensure any references in the same file
that populate or export the provider use MAPS_PROVIDER so the value read
alongside MAPS_API_KEY is passed into the container.
davidbeig
force-pushed
the
feat/decidim_install
branch
from
079ac30 to
0319ea1
Compare
4 participants
This PR closes #113 .
We are adding a script that enables the creation of a Decidim instance based on the docker images built in this repository. The user will be guided through some questions on how to properly configure the instance, and the script itself will take care of the necessary dependencies (such as the database, where the storage is going to be located, etc).
Some improvements:
This will generate extra-security on the download of the shell script and the files it uses, giving the final user more security.
What we still need
How to test
Using the released zip.
sudo curl -s https://raw.githubusercontent.com/decidim/docker/refs/heads/feat/decidim_install/install/install.sh | bashWhile developing we might not have access to the released zip, the way to test it is to zip ourselfs the script folder and push it to the server.
Later on, we need the deploy.zip to be under the
/tmp/decidim-docker-files/deploy.zip. It's a bit messy right now.Also, if you want to try it locally you can use
multipassSummary by CodeRabbit
New Features
Documentation
Refactor