Conversation
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
079ac30 to
0319ea1
Compare
microstudi
left a comment
microstudi
left a comment
There was a problem hiding this comment.
LGTM, I'll pass the token to @decidim/maintainers
| fi | ||
|
|
||
| # Simple health check for Rails server | ||
| until docker exec decidim curl -s http://localhost:3000/up >/dev/null; do |
There was a problem hiding this comment.
It stucks here:
There isn't a container called decidim. It should be decidim-app-1
root@decidim-docker-installation-01:/opt/decidim# docker exec decidim curl -s http://localhost:3000/up
Error response from daemon: No such container: decidim
root@decidim-docker-installation-01:/opt/decidim#
root@decidim-docker-installation-01:/opt/decidim# docker exec decidim-app-1 curl -s http://localhost:3000/up
<!DOCTYPE html><html><body style="background-color: green"></body></html>root@decidim-docker-installation-01:/opt/decidim#
| until docker exec decidim curl -s http://localhost:3000/up >/dev/null; do | |
| until docker exec decidim-app-1 curl -s http://localhost:3000/up >/dev/null; do |
There was a problem hiding this comment.
Mind that the app is fully loaded when it arrives here:
root@decidim-docker:/opt/decidim# docker compose logs -f
(...)
app-1 | -> 0.0041s
app-1 | == 20260706094051 RenameBlogPostsEndorsementsCountToLikes: migrated (0.0042s) =
app-1 |
app-1 | + echo ✅ Migrations are all up!
app-1 | + echo 🚀 bin/rails server -b 0.0.0.0
app-1 | + exec bin/rails server -b 0.0.0.0
app-1 | ✅ Migrations are all up!
app-1 | 🚀 bin/rails server -b 0.0.0.0
app-1 | => Booting Puma
app-1 | => Rails 7.2.3.1 application starting in production
app-1 | => Run bin/rails server --help for more startup options
app-1 | [1] Puma starting in cluster mode...
app-1 | [1] * Puma version: 6.6.1 ("Return to Forever")
app-1 | [1] * Ruby version: ruby 3.3.4 (2024-07-09 revision be1089c8ec) +YJIT [x86_64-linux]
app-1 | [1] * Min threads: 3
app-1 | [1] * Max threads: 3
app-1 | [1] * Environment: production
app-1 | [1] * Master PID: 1
app-1 | [1] * Workers: 2
app-1 | [1] * Restarts: (✔) hot (✖) phased (✖) refork
app-1 | [1] * Preloading application
app-1 | [1] * Listening on http://0.0.0.0:3000
app-1 | [1] Use Ctrl-C to stop
app-1 | [1] - Worker 0 (PID: 1417) booted in 0.01s, phase: 0
app-1 | [1] - Worker 1 (PID: 1428) booted in 0.0s, phase: 0
| - migrations_data:/code/db/migrate | ||
| labels: | ||
| - "traefik.enable=true" | ||
| - "traefik.http.routers.app.rule=Host(`$$DECIDIM_DOMAIN`)" |
There was a problem hiding this comment.
| - "traefik.http.routers.app.rule=Host(`$$DECIDIM_DOMAIN`)" | |
| - "traefik.http.routers.app.rule=Host(`$DECIDIM_DOMAIN`)" |
There was a problem hiding this comment.
With the $$ the domain isn't correctly added here so the SSL certificate isn't configured correctly
| SYSTEM_PASSWORD="$(openssl rand -hex 12)" | ||
|
|
||
| docker exec -ti \ | ||
| decidim \ |
There was a problem hiding this comment.
| decidim \ | |
| decidim-app-1 \ |
| echo "📥 Downloading the necessary installation files." | ||
| 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" |
There was a problem hiding this comment.
This is a bit of a quality of developer experience that I needed to do to check the fixes that I'm proposing. I don't know if you had any other way of handling this flow.
What I did to check my changes are:
(locally) cd install && zip -r deploy.zip * && scp deploy.zip root@decidim-docker.test.decidim.org: && ssh root@decidim-docker.test.decidim.org
(remote) mkdir /tmp/decidim-docker-files && cp deploy.zip /tmp/decidim-docker-files/ && unzip deploy.zip && bash install.sh
Or do you have a better way for this?
| echo "📥 Downloading the necessary installation files." | |
| 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" | |
| if [ ! -f $TMP/deploy.zip ]; then | |
| echo "📥 Downloading the necessary installation files." | |
| 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" | |
| else | |
| echo "📦 Using local deploy.zip for installation files." | |
| fi |
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