♻️ (deploy) harmonize prod deployment layout under deploy/#2425
Open
Nastaliss wants to merge 1 commit into
Open
♻️ (deploy) harmonize prod deployment layout under deploy/#2425Nastaliss wants to merge 1 commit into
Nastaliss wants to merge 1 commit into
Conversation
Move helm chart, prod compose examples and buildpack scripts under
deploy/{kubernetes,docker,paas}/. Wire THEME_CUSTOMIZATION_JSON
directly in settings so the env var works on Docker and k8s without
the Scalingo buildpack pivot to a file.
Nastaliss
force-pushed
the
harmonize-suitenumerique-os
branch
from
a1b1fc9 to
d0230e3
Compare
WalkthroughThis PR centralizes deployment artifacts under deploy/ (kubernetes/helm, paas, docker), updates CI workflows, Procfile, Tiltfile, buildpack scripts, and documentation to use the new paths, and adds THEME_CUSTOMIZATION_JSON support (settings, loader with caching/error handling, tests, and docs). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/customization.md (1)
140-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep settings snippets consistent with the new dual-input model.
After introducing
THEME_CUSTOMIZATION_JSONsupport (Line 96), the Footer/Translations/Waffle settings blocks still document onlyTHEME_CUSTOMIZATION_FILE_PATH. Align those sections with the same “file or inline JSON” pattern to avoid operator confusion.Also applies to: 159-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/customization.md` around lines 140 - 183, Update the Footer, Custom Translations, and Waffle settings sections to reflect the new dual-input model by documenting both THEME_CUSTOMIZATION_FILE_PATH and THEME_CUSTOMIZATION_JSON (same pattern introduced earlier at THEME_CUSTOMIZATION_JSON) wherever only THEME_CUSTOMIZATION_FILE_PATH is shown; change the settings snippets and explanatory text in the Footer, Translations, and Waffle blocks to describe "file or inline JSON" usage and show the same example/format used for the earlier dual-input example so operators can supply either an env var pointing to a file or an inline JSON string..github/workflows/release-helm-chart.yaml (1)
4-15: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSerialize chart publication jobs.
This workflow writes release artifacts with
contents: write, so two pushes landing close together can race and overwrite the generated Helm index or chart publication state. Add a workflow- or job-levelconcurrencygroup before this starts publishing.Suggested change
on: push: paths: - deploy/kubernetes/helm/impress/** + +concurrency: + group: release-chart-${{ github.ref }} + cancel-in-progress: false jobs: release:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/release-helm-chart.yaml around lines 4 - 15, The release job can race when multiple pushes run concurrently because it writes release artifacts; add a concurrency group to serialize publication by adding a top-level or job-level concurrency entry (for the job named "release") with a stable group identifier (e.g., "release-helm-chart-{{ github.ref }}") and configure cancel-in-progress as appropriate so only one publish run proceeds at a time, preventing simultaneous writes under the permissions: contents: write block.Source: Linters/SAST tools
deploy/docker/examples/keycloak/README.md (1)
58-64:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the same hostname in the post-setup URL.
The walkthrough configures Keycloak on
id.yourdomain.tld, but Line 64 says the instance is available onhttps://doc.yourdomain.tld. That sends readers to the wrong endpoint after setup.Suggested change
-Your keycloak instance is now available on https://doc.yourdomain.tld +Your keycloak instance is now available on https://id.yourdomain.tld🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/docker/examples/keycloak/README.md` around lines 58 - 64, Update the post-setup URL in the Step 4 "Start the service" section of the Keycloak README so it matches the hostname used earlier in the walkthrough (change the incorrect https://doc.yourdomain.tld to https://id.yourdomain.tld); modify the Step 4 paragraph in deploy/docker/examples/keycloak/README.md (the "Start the service" block) so the displayed final URL consistently uses id.yourdomain.tld.docs/installation/compose.md (1)
42-45:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the nginx-proxy template URL to the new
deploy/layout.Line 44 still downloads the template from the old
docker/files/...location, so this example will 404 once the old tree is removed.Suggested fix
-curl -o default.conf.template https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/docker/files/production/etc/nginx/conf.d/default.conf.template +curl -o default.conf.template https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/files/production/etc/nginx/conf.d/default.conf.template🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/installation/compose.md` around lines 42 - 45, Update the example curl URL to point at the new deploy/ layout instead of the old docker/files/... path: change the download URL used in the nginx-proxy example (the curl command that saves default.conf.template) to the corresponding raw URL under refs/heads/main/deploy/etc/nginx/conf.d/default.conf.template so it won't 404 after the old tree is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/docker/examples/keycloak/README.md`:
- Around line 10-14: Update the README snippet so the env.d directory exists
before the curl commands: replace or augment the existing "mkdir keycloak" step
so it creates "keycloak/env.d" (e.g., use a single command that creates parent
and env.d) so the subsequent curl lines that write to
"keycloak/env.d/kc_postgresql" and "keycloak/env.d/keycloak" will not fail on a
clean machine.
In `@deploy/docker/README.md`:
- Around line 25-33: The README's curl targets for the compose files are
pointing to nonexistent URLs causing 404s; update the curl lines that fetch
"compose.yml" and the example compose files so they reference the actual raw
file locations and correct filenames/branch (replace the broken refs/heads/main
paths and/or ".yml" names with the real raw.githubusercontent.com paths and
correct filenames like compose.yaml for the main compose and the
examples/keycloak/compose.yaml and examples/minio/compose.yaml), verify each URL
returns 200 and commit the corrected README.md.
In `@deploy/paas/README.md`:
- Around line 7-13: The fenced code block listing the directory contents lacks a
language tag and triggers MD040; update the README fenced block that contains
the lines starting with "deploy/paas/" and the filenames (e.g.,
buildpack_postcompile.sh, buildpack_postfrontend.sh, buildpack_start.sh,
servers.conf.erb) by adding the language tag "text" immediately after the
opening triple backticks (i.e., replace ``` with ```text) and keep the closing
triple backticks unchanged.
In `@src/backend/core/api/viewsets.py`:
- Around line 3111-3123: After json.loads(settings.THEME_CUSTOMIZATION_JSON)
succeeds, validate that the resulting theme_customization is a dict (JSON
object) before using or caching it; if it's not a dict, log an error (including
the unexpected type), return {} and do not call cache.set with the invalid
payload. Update the block around theme_customization, json.loads, cache.set and
cache_key to perform this type check and early return.
- Around line 3106-3121: The cache key is fixed ("theme_customization_env") so
different deployments or updates to settings.THEME_CUSTOMIZATION_JSON can return
stale data; update the cache key used around cache_key and theme_customization
to include a unique per-config identifier (e.g., a hash/digest of
settings.THEME_CUSTOMIZATION_JSON or a deployment/version setting) so the key
becomes deterministic per JSON value, then use that composite key when reading
and writing the cache and keep the existing JSON parsing and error handling
(settings.THEME_CUSTOMIZATION_JSON, settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT,
theme_customization).
In `@src/backend/core/tests/test_api_config.py`:
- Around line 203-262: Add a new test (e.g.,
test_api_config_theme_customization_env_var_cache_update) that verifies cached
reads update when THEME_CUSTOMIZATION_JSON changes: create an APIClient, set
settings.THEME_CUSTOMIZATION_JSON to an initial JSON (e.g., {"value":"one"}) and
call client.get("/api/v1.0/config/") asserting theme_customization ==
{"value":"one"}, then without calling cache.clear() change
settings.THEME_CUSTOMIZATION_JSON to a different JSON (e.g., {"value":"two"})
using Django's override_settings context manager or monkeypatch, call
client.get("/api/v1.0/config/") again and assert the response's
theme_customization == {"value":"two"} to ensure the code that reads
THEME_CUSTOMIZATION_JSON (used by the existing tests like
test_api_config_with_theme_customization_from_env_var) picks up env-value
changes across cached reads.
---
Outside diff comments:
In @.github/workflows/release-helm-chart.yaml:
- Around line 4-15: The release job can race when multiple pushes run
concurrently because it writes release artifacts; add a concurrency group to
serialize publication by adding a top-level or job-level concurrency entry (for
the job named "release") with a stable group identifier (e.g.,
"release-helm-chart-{{ github.ref }}") and configure cancel-in-progress as
appropriate so only one publish run proceeds at a time, preventing simultaneous
writes under the permissions: contents: write block.
In `@deploy/docker/examples/keycloak/README.md`:
- Around line 58-64: Update the post-setup URL in the Step 4 "Start the service"
section of the Keycloak README so it matches the hostname used earlier in the
walkthrough (change the incorrect https://doc.yourdomain.tld to
https://id.yourdomain.tld); modify the Step 4 paragraph in
deploy/docker/examples/keycloak/README.md (the "Start the service" block) so the
displayed final URL consistently uses id.yourdomain.tld.
In `@docs/customization.md`:
- Around line 140-183: Update the Footer, Custom Translations, and Waffle
settings sections to reflect the new dual-input model by documenting both
THEME_CUSTOMIZATION_FILE_PATH and THEME_CUSTOMIZATION_JSON (same pattern
introduced earlier at THEME_CUSTOMIZATION_JSON) wherever only
THEME_CUSTOMIZATION_FILE_PATH is shown; change the settings snippets and
explanatory text in the Footer, Translations, and Waffle blocks to describe
"file or inline JSON" usage and show the same example/format used for the
earlier dual-input example so operators can supply either an env var pointing to
a file or an inline JSON string.
In `@docs/installation/compose.md`:
- Around line 42-45: Update the example curl URL to point at the new deploy/
layout instead of the old docker/files/... path: change the download URL used in
the nginx-proxy example (the curl command that saves default.conf.template) to
the corresponding raw URL under
refs/heads/main/deploy/etc/nginx/conf.d/default.conf.template so it won't 404
after the old tree is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41d41730-a16e-41cc-8513-6fb2200735b6
📒 Files selected for processing (62)
.github/workflows/helmfile-linter.yaml.github/workflows/release-helm-chart.yamlProcfilebin/Tiltfiledeploy/docker/README.mddeploy/docker/compose.ymldeploy/docker/examples/keycloak/README.mddeploy/docker/examples/keycloak/compose.yamldeploy/docker/examples/minio/README.mddeploy/docker/examples/minio/compose.yamldeploy/docker/examples/nginx-proxy/README.mddeploy/docker/examples/nginx-proxy/compose.yamldeploy/kubernetes/helm/env.d/dev/configuration/theme/demo.jsondeploy/kubernetes/helm/env.d/dev/values.dev-backend.yaml.gotmpldeploy/kubernetes/helm/env.d/dev/values.impress.yaml.gotmpldeploy/kubernetes/helm/env.d/feature/values.dev-backend.yaml.gotmpldeploy/kubernetes/helm/env.d/feature/values.impress.yaml.gotmpldeploy/kubernetes/helm/helmfile.yaml.gotmpldeploy/kubernetes/helm/impress/Chart.yamldeploy/kubernetes/helm/impress/README.mddeploy/kubernetes/helm/impress/generate-readme.shdeploy/kubernetes/helm/impress/templates/_helpers.tpldeploy/kubernetes/helm/impress/templates/backend_cronjob_list.yamldeploy/kubernetes/helm/impress/templates/backend_deployment.yamldeploy/kubernetes/helm/impress/templates/backend_job.ymldeploy/kubernetes/helm/impress/templates/backend_job_createsuperuser.yamldeploy/kubernetes/helm/impress/templates/backend_job_migrate.yamldeploy/kubernetes/helm/impress/templates/backend_svc.yamldeploy/kubernetes/helm/impress/templates/celery_worker_deployment.yamldeploy/kubernetes/helm/impress/templates/docspec_deployment.yamldeploy/kubernetes/helm/impress/templates/docspec_svc.yamldeploy/kubernetes/helm/impress/templates/frontend_deployment.yamldeploy/kubernetes/helm/impress/templates/frontend_svc.yamldeploy/kubernetes/helm/impress/templates/ingress-redirects.yamldeploy/kubernetes/helm/impress/templates/ingress.yamldeploy/kubernetes/helm/impress/templates/ingress_admin.yamldeploy/kubernetes/helm/impress/templates/ingress_collaboration_api.yamldeploy/kubernetes/helm/impress/templates/ingress_collaboration_ws.yamldeploy/kubernetes/helm/impress/templates/ingress_media.yamldeploy/kubernetes/helm/impress/templates/ingress_posthog.yamldeploy/kubernetes/helm/impress/templates/ingress_posthog_assets.yamldeploy/kubernetes/helm/impress/templates/media_svc.yamldeploy/kubernetes/helm/impress/templates/posthog_assets_svc.yamldeploy/kubernetes/helm/impress/templates/posthog_svc.yamldeploy/kubernetes/helm/impress/templates/theme_customization_file_cm.yamldeploy/kubernetes/helm/impress/templates/yprovider_deployment.yamldeploy/kubernetes/helm/impress/templates/yprovider_deployment_converter.yamldeploy/kubernetes/helm/impress/templates/yprovider_svc.yamldeploy/kubernetes/helm/impress/templates/yprovider_svc_converter.yamldeploy/kubernetes/helm/impress/values.yamldeploy/paas/README.mddeploy/paas/buildpack_postcompile.shdeploy/paas/buildpack_postfrontend.shdeploy/paas/buildpack_start.shdocs/customization.mddocs/env.mddocs/installation/compose.mddocs/installation/scalingo.mddocs/release.mdsrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/backend/impress/settings.py
Comment on lines
10
to
14
| ```bash | ||
| mkdir keycloak | ||
| curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/docs/examples/compose/keycloak/compose.yaml | ||
| curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml | ||
| curl -o keycloak/env.d/kc_postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/kc_postgresql | ||
| curl -o keycloak/env.d/keycloak https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/keycloak |
There was a problem hiding this comment.
Create keycloak/env.d before downloading the env files.
Step 1 only creates keycloak/, but Lines 13-14 write into keycloak/env.d/. On a clean machine those curl -o commands fail because the parent directory does not exist.
Suggested change
-mkdir keycloak
+mkdir -p keycloak/env.d
curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml
curl -o keycloak/env.d/kc_postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/kc_postgresql
curl -o keycloak/env.d/keycloak https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/keycloak📝 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.
Suggested change
| ```bash | |
| mkdir keycloak | |
| curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/docs/examples/compose/keycloak/compose.yaml | |
| curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml | |
| curl -o keycloak/env.d/kc_postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/kc_postgresql | |
| curl -o keycloak/env.d/keycloak https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/keycloak | |
| mkdir -p keycloak/env.d | |
| curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml | |
| curl -o keycloak/env.d/kc_postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/kc_postgresql | |
| curl -o keycloak/env.d/keycloak https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/keycloak |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/docker/examples/keycloak/README.md` around lines 10 - 14, Update the
README snippet so the env.d directory exists before the curl commands: replace
or augment the existing "mkdir keycloak" step so it creates "keycloak/env.d"
(e.g., use a single command that creates parent and env.d) so the subsequent
curl lines that write to "keycloak/env.d/kc_postgresql" and
"keycloak/env.d/keycloak" will not fail on a clean machine.
Comment on lines
+25
to
+33
| ```bash | ||
| mkdir -p docs/env.d && cd docs | ||
|
|
||
| # Fetch the compose file and example env files | ||
| curl -o compose.yml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/compose.yml | ||
| curl -o env.d/common https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/common | ||
| curl -o env.d/backend https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/backend | ||
| curl -o env.d/yprovider https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/yprovider | ||
| curl -o env.d/postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/postgresql |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
urls=(
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/compose.yml"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/common"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/backend"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/yprovider"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/postgresql"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml"
"https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/minio/compose.yaml"
)
for url in "${urls[@]}"; do
code="$(curl -sS -L -o /dev/null -w '%{http_code}' "$url")"
printf '%s -> %s\n' "$url" "$code"
doneRepository: suitenumerique/docs
Length of output: 839
Fix broken compose download URLs (404s).
In deploy/docker/README.md (lines 25-33), the raw.githubusercontent.com/.../refs/heads/main/... URL format works for env.d/production.dist/* (200), but the compose files return 404, so the bootstrap will fail:
deploy/docker/compose.yml-> 404deploy/docker/examples/keycloak/compose.yaml-> 404deploy/docker/examples/minio/compose.yaml-> 404
Update the paths/filenames (or branch) for those compose artifacts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/docker/README.md` around lines 25 - 33, The README's curl targets for
the compose files are pointing to nonexistent URLs causing 404s; update the curl
lines that fetch "compose.yml" and the example compose files so they reference
the actual raw file locations and correct filenames/branch (replace the broken
refs/heads/main paths and/or ".yml" names with the real
raw.githubusercontent.com paths and correct filenames like compose.yaml for the
main compose and the examples/keycloak/compose.yaml and
examples/minio/compose.yaml), verify each URL returns 200 and commit the
corrected README.md.
Comment on lines
+7
to
+13
| ``` | ||
| deploy/paas/ | ||
| ├── buildpack_postcompile.sh # Build-time: cleanup unused files to reduce slug size | ||
| ├── buildpack_postfrontend.sh # Build-time: assemble frontend/backend/nginx into slug | ||
| ├── buildpack_start.sh # Runtime: starts uvicorn + y-provider + nginx | ||
| └── servers.conf.erb # Nginx routing template (ERB → consumed by buildpack) | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the layout fence.
The bare fenced block trips markdownlint (MD040). text is enough here.
Suggested fix
-```
+```text
deploy/paas/
├── buildpack_postcompile.sh # Build-time: cleanup unused files to reduce slug size
├── buildpack_postfrontend.sh # Build-time: assemble frontend/backend/nginx into slug
├── buildpack_start.sh # Runtime: starts uvicorn + y-provider + nginx
└── servers.conf.erb # Nginx routing template (ERB → consumed by buildpack)
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/paas/README.md` around lines 7 - 13, The fenced code block listing the
directory contents lacks a language tag and triggers MD040; update the README
fenced block that contains the lines starting with "deploy/paas/" and the
filenames (e.g., buildpack_postcompile.sh, buildpack_postfrontend.sh,
buildpack_start.sh, servers.conf.erb) by adding the language tag "text"
immediately after the opening triple backticks (i.e., replace ``` with ```text)
and keep the closing triple backticks unchanged.
Source: Linters/SAST tools
Comment on lines
+3106
to
+3121
| cache_key = "theme_customization_env" | ||
| theme_customization = cache.get(cache_key, {}) | ||
| if theme_customization: | ||
| return theme_customization | ||
|
|
||
| try: | ||
| theme_customization = json.loads(settings.THEME_CUSTOMIZATION_JSON) | ||
| except json.JSONDecodeError: | ||
| logger.error("THEME_CUSTOMIZATION_JSON is not a valid JSON") | ||
| return {} | ||
|
|
||
| cache.set( | ||
| cache_key, | ||
| theme_customization, | ||
| settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT, | ||
| ) |
There was a problem hiding this comment.
Cache key does not vary with env value, causing stale theme after config changes.
Line 3106 uses a fixed key (theme_customization_env). With shared Redis, a deployment that changes THEME_CUSTOMIZATION_JSON can still return the previous cached theme until timeout.
💡 Suggested fix
+import hashlib
...
- cache_key = "theme_customization_env"
+ theme_json = settings.THEME_CUSTOMIZATION_JSON
+ cache_key = f"theme_customization_env_{hashlib.sha256(theme_json.encode()).hexdigest()}"
theme_customization = cache.get(cache_key, {})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/viewsets.py` around lines 3106 - 3121, The cache key is
fixed ("theme_customization_env") so different deployments or updates to
settings.THEME_CUSTOMIZATION_JSON can return stale data; update the cache key
used around cache_key and theme_customization to include a unique per-config
identifier (e.g., a hash/digest of settings.THEME_CUSTOMIZATION_JSON or a
deployment/version setting) so the key becomes deterministic per JSON value,
then use that composite key when reading and writing the cache and keep the
existing JSON parsing and error handling (settings.THEME_CUSTOMIZATION_JSON,
settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT, theme_customization).
Comment on lines
+3111
to
+3123
| try: | ||
| theme_customization = json.loads(settings.THEME_CUSTOMIZATION_JSON) | ||
| except json.JSONDecodeError: | ||
| logger.error("THEME_CUSTOMIZATION_JSON is not a valid JSON") | ||
| return {} | ||
|
|
||
| cache.set( | ||
| cache_key, | ||
| theme_customization, | ||
| settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT, | ||
| ) | ||
| return theme_customization | ||
|
|
There was a problem hiding this comment.
Validate that THEME_CUSTOMIZATION_JSON decodes to a JSON object.
At Line 3112, json.loads accepts scalar/array JSON too. That can return a non-dict payload in theme_customization, breaking the response contract expected by consumers.
💡 Suggested fix
try:
theme_customization = json.loads(settings.THEME_CUSTOMIZATION_JSON)
except json.JSONDecodeError:
logger.error("THEME_CUSTOMIZATION_JSON is not a valid JSON")
return {}
+ if not isinstance(theme_customization, dict):
+ logger.error("THEME_CUSTOMIZATION_JSON must decode to a JSON object")
+ return {}📝 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.
Suggested change
| try: | |
| theme_customization = json.loads(settings.THEME_CUSTOMIZATION_JSON) | |
| except json.JSONDecodeError: | |
| logger.error("THEME_CUSTOMIZATION_JSON is not a valid JSON") | |
| return {} | |
| cache.set( | |
| cache_key, | |
| theme_customization, | |
| settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT, | |
| ) | |
| return theme_customization | |
| try: | |
| theme_customization = json.loads(settings.THEME_CUSTOMIZATION_JSON) | |
| except json.JSONDecodeError: | |
| logger.error("THEME_CUSTOMIZATION_JSON is not a valid JSON") | |
| return {} | |
| if not isinstance(theme_customization, dict): | |
| logger.error("THEME_CUSTOMIZATION_JSON must decode to a JSON object") | |
| return {} | |
| cache.set( | |
| cache_key, | |
| theme_customization, | |
| settings.THEME_CUSTOMIZATION_CACHE_TIMEOUT, | |
| ) | |
| return theme_customization |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/api/viewsets.py` around lines 3111 - 3123, After
json.loads(settings.THEME_CUSTOMIZATION_JSON) succeeds, validate that the
resulting theme_customization is a dict (JSON object) before using or caching
it; if it's not a dict, log an error (including the unexpected type), return {}
and do not call cache.set with the invalid payload. Update the block around
theme_customization, json.loads, cache.set and cache_key to perform this type
check and early return.
Comment on lines
+203
to
+262
| @override_settings( | ||
| THEME_CUSTOMIZATION_JSON=json.dumps( | ||
| {"colors": {"primary": "#abcdef", "secondary": "#fedcba"}} | ||
| ), | ||
| ) | ||
| @pytest.mark.parametrize("is_authenticated", [False, True]) | ||
| def test_api_config_with_theme_customization_from_env_var(is_authenticated): | ||
| """Theme customization can be provided inline via THEME_CUSTOMIZATION_JSON.""" | ||
| cache.clear() | ||
| client = APIClient() | ||
|
|
||
| if is_authenticated: | ||
| user = factories.UserFactory() | ||
| client.force_login(user) | ||
|
|
||
| response = client.get("/api/v1.0/config/") | ||
| assert response.status_code == HTTP_200_OK | ||
| assert response.json()["theme_customization"] == { | ||
| "colors": {"primary": "#abcdef", "secondary": "#fedcba"}, | ||
| } | ||
|
|
||
|
|
||
| @override_settings(THEME_CUSTOMIZATION_JSON="not a json") | ||
| @pytest.mark.parametrize("is_authenticated", [False, True]) | ||
| def test_api_config_with_invalid_theme_customization_env_var(is_authenticated): | ||
| """An invalid THEME_CUSTOMIZATION_JSON value should yield an empty dict.""" | ||
| cache.clear() | ||
| client = APIClient() | ||
|
|
||
| if is_authenticated: | ||
| user = factories.UserFactory() | ||
| client.force_login(user) | ||
|
|
||
| response = client.get("/api/v1.0/config/") | ||
| assert response.status_code == HTTP_200_OK | ||
| assert response.json()["theme_customization"] == {} | ||
|
|
||
|
|
||
| @override_settings( | ||
| THEME_CUSTOMIZATION_JSON=json.dumps({"from": "env"}), | ||
| THEME_CUSTOMIZATION_FILE_PATH="/configuration/theme/default.json", | ||
| ) | ||
| @pytest.mark.parametrize("is_authenticated", [False, True]) | ||
| def test_api_config_theme_customization_env_var_takes_precedence(is_authenticated, fs): | ||
| """THEME_CUSTOMIZATION_JSON takes precedence over THEME_CUSTOMIZATION_FILE_PATH.""" | ||
| cache.clear() | ||
| fs.create_file( | ||
| "/configuration/theme/default.json", | ||
| contents=json.dumps({"from": "file"}), | ||
| ) | ||
| client = APIClient() | ||
|
|
||
| if is_authenticated: | ||
| user = factories.UserFactory() | ||
| client.force_login(user) | ||
|
|
||
| response = client.get("/api/v1.0/config/") | ||
| assert response.status_code == HTTP_200_OK | ||
| assert response.json()["theme_customization"] == {"from": "env"} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add regression coverage for env-value change across cached reads.
Current tests don’t catch stale-cache behavior when THEME_CUSTOMIZATION_JSON changes while cache persists. Add one test that performs two calls with different env values (without cache.clear() between them) and asserts the second response reflects the new value.
🧰 Tools
🪛 ast-grep (0.43.0)
[info] 203-205: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{"colors": {"primary": "#abcdef", "secondary": "#fedcba"}}
)
Note: Security best practice.
(use-jsonify)
[info] 241-241: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"from": "env"})
Note: Security best practice.
(use-jsonify)
[info] 250-250: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"from": "file"})
Note: Security best practice.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/core/tests/test_api_config.py` around lines 203 - 262, Add a new
test (e.g., test_api_config_theme_customization_env_var_cache_update) that
verifies cached reads update when THEME_CUSTOMIZATION_JSON changes: create an
APIClient, set settings.THEME_CUSTOMIZATION_JSON to an initial JSON (e.g.,
{"value":"one"}) and call client.get("/api/v1.0/config/") asserting
theme_customization == {"value":"one"}, then without calling cache.clear()
change settings.THEME_CUSTOMIZATION_JSON to a different JSON (e.g.,
{"value":"two"}) using Django's override_settings context manager or
monkeypatch, call client.get("/api/v1.0/config/") again and assert the
response's theme_customization == {"value":"two"} to ensure the code that reads
THEME_CUSTOMIZATION_JSON (used by the existing tests like
test_api_config_with_theme_customization_from_env_var) picks up env-value
changes across cached reads.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
deploy/docker/examples/keycloak/README.md (1)
11-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate
keycloak/env.dbefore downloading the env files.Line 11 only creates
keycloak/, but Lines 13-14 write intokeycloak/env.d/. On a clean machine thosecurl -ocommands will fail because the parent directory does not exist.📝 Proposed fix
-mkdir keycloak +mkdir -p keycloak/env.d curl -o keycloak/compose.yaml https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/deploy/docker/examples/keycloak/compose.yaml curl -o keycloak/env.d/kc_postgresql https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/kc_postgresql curl -o keycloak/env.d/keycloak https://raw.githubusercontent.com/suitenumerique/docs/refs/heads/main/env.d/production.dist/keycloak🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/docker/examples/keycloak/README.md` around lines 11 - 14, The README's setup sequence creates only the keycloak directory but then writes files into keycloak/env.d, causing failures on a clean machine; update the steps so you create the env.d directory first (e.g., run mkdir -p keycloak/env.d or add a mkdir keycloak/env.d before the curl -o keycloak/env.d/... commands) so the curl commands that write kc_postgresql and keycloak env files succeed; ensure the updated instructions reference the existing mkdir keycloak and the subsequent curl -o keycloak/env.d/kc_postgresql and curl -o keycloak/env.d/keycloak commands.deploy/docker/README.md (1)
29-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix broken compose download URLs (404s).
Based on previous verification, the
compose.ymlURL returns 404. The compose files at these raw.githubusercontent.com URLs are returning 404 errors, which will cause the bootstrap to fail:
deploy/docker/compose.yml→ 404deploy/docker/examples/keycloak/compose.yaml→ 404deploy/docker/examples/minio/compose.yaml→ 404Verify the actual filenames and paths in the repository. The extension might be
.yamlinstead of.yml, or the files may not exist at these locations yet on the main branch.Run the following script to verify which compose files exist and their correct paths:
#!/bin/bash # Description: Find all compose files under deploy/docker/ fd -t f -e yml -e yaml 'compose' deploy/docker/ # Also check if these specific files exist echo "=== Checking specific paths ===" for file in deploy/docker/compose.yml deploy/docker/compose.yaml deploy/docker/examples/keycloak/compose.yaml deploy/docker/examples/minio/compose.yaml; do if [ -f "$file" ]; then echo "EXISTS: $file" else echo "MISSING: $file" fi done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/docker/README.md` around lines 29 - 33, The curl URLs in the README are pointing to missing files (404); verify the actual filenames under deploy/docker (they may be .yaml not .yml or located under examples/) and update the raw.githubusercontent.com links accordingly: replace compose.yml with the correct filename (e.g., compose.yaml) and correct the example paths (examples/keycloak/compose.yaml and examples/minio/compose.yaml) to match the repository, or point to existing alternatives; ensure each curl target name (compose.yml, env.d/common, env.d/backend, env.d/yprovider, env.d/postgresql) matches the remote file name and update the README curl commands to the exact raw URLs found.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@deploy/docker/examples/keycloak/README.md`:
- Around line 11-14: The README's setup sequence creates only the keycloak
directory but then writes files into keycloak/env.d, causing failures on a clean
machine; update the steps so you create the env.d directory first (e.g., run
mkdir -p keycloak/env.d or add a mkdir keycloak/env.d before the curl -o
keycloak/env.d/... commands) so the curl commands that write kc_postgresql and
keycloak env files succeed; ensure the updated instructions reference the
existing mkdir keycloak and the subsequent curl -o keycloak/env.d/kc_postgresql
and curl -o keycloak/env.d/keycloak commands.
In `@deploy/docker/README.md`:
- Around line 29-33: The curl URLs in the README are pointing to missing files
(404); verify the actual filenames under deploy/docker (they may be .yaml not
.yml or located under examples/) and update the raw.githubusercontent.com links
accordingly: replace compose.yml with the correct filename (e.g., compose.yaml)
and correct the example paths (examples/keycloak/compose.yaml and
examples/minio/compose.yaml) to match the repository, or point to existing
alternatives; ensure each curl target name (compose.yml, env.d/common,
env.d/backend, env.d/yprovider, env.d/postgresql) matches the remote file name
and update the README curl commands to the exact raw URLs found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba47e54b-179c-451e-b894-03eacf956267
📒 Files selected for processing (63)
.github/workflows/helmfile-linter.yaml.github/workflows/release-helm-chart.yamlCHANGELOG.mdProcfilebin/Tiltfiledeploy/docker/README.mddeploy/docker/compose.ymldeploy/docker/examples/keycloak/README.mddeploy/docker/examples/keycloak/compose.yamldeploy/docker/examples/minio/README.mddeploy/docker/examples/minio/compose.yamldeploy/docker/examples/nginx-proxy/README.mddeploy/docker/examples/nginx-proxy/compose.yamldeploy/kubernetes/helm/env.d/dev/configuration/theme/demo.jsondeploy/kubernetes/helm/env.d/dev/values.dev-backend.yaml.gotmpldeploy/kubernetes/helm/env.d/dev/values.impress.yaml.gotmpldeploy/kubernetes/helm/env.d/feature/values.dev-backend.yaml.gotmpldeploy/kubernetes/helm/env.d/feature/values.impress.yaml.gotmpldeploy/kubernetes/helm/helmfile.yaml.gotmpldeploy/kubernetes/helm/impress/Chart.yamldeploy/kubernetes/helm/impress/README.mddeploy/kubernetes/helm/impress/generate-readme.shdeploy/kubernetes/helm/impress/templates/_helpers.tpldeploy/kubernetes/helm/impress/templates/backend_cronjob_list.yamldeploy/kubernetes/helm/impress/templates/backend_deployment.yamldeploy/kubernetes/helm/impress/templates/backend_job.ymldeploy/kubernetes/helm/impress/templates/backend_job_createsuperuser.yamldeploy/kubernetes/helm/impress/templates/backend_job_migrate.yamldeploy/kubernetes/helm/impress/templates/backend_svc.yamldeploy/kubernetes/helm/impress/templates/celery_worker_deployment.yamldeploy/kubernetes/helm/impress/templates/docspec_deployment.yamldeploy/kubernetes/helm/impress/templates/docspec_svc.yamldeploy/kubernetes/helm/impress/templates/frontend_deployment.yamldeploy/kubernetes/helm/impress/templates/frontend_svc.yamldeploy/kubernetes/helm/impress/templates/ingress-redirects.yamldeploy/kubernetes/helm/impress/templates/ingress.yamldeploy/kubernetes/helm/impress/templates/ingress_admin.yamldeploy/kubernetes/helm/impress/templates/ingress_collaboration_api.yamldeploy/kubernetes/helm/impress/templates/ingress_collaboration_ws.yamldeploy/kubernetes/helm/impress/templates/ingress_media.yamldeploy/kubernetes/helm/impress/templates/ingress_posthog.yamldeploy/kubernetes/helm/impress/templates/ingress_posthog_assets.yamldeploy/kubernetes/helm/impress/templates/media_svc.yamldeploy/kubernetes/helm/impress/templates/posthog_assets_svc.yamldeploy/kubernetes/helm/impress/templates/posthog_svc.yamldeploy/kubernetes/helm/impress/templates/theme_customization_file_cm.yamldeploy/kubernetes/helm/impress/templates/yprovider_deployment.yamldeploy/kubernetes/helm/impress/templates/yprovider_deployment_converter.yamldeploy/kubernetes/helm/impress/templates/yprovider_svc.yamldeploy/kubernetes/helm/impress/templates/yprovider_svc_converter.yamldeploy/kubernetes/helm/impress/values.yamldeploy/paas/README.mddeploy/paas/buildpack_postcompile.shdeploy/paas/buildpack_postfrontend.shdeploy/paas/buildpack_start.shdocs/customization.mddocs/env.mddocs/installation/compose.mddocs/installation/scalingo.mddocs/release.mdsrc/backend/core/api/viewsets.pysrc/backend/core/tests/test_api_config.pysrc/backend/impress/settings.py
Nastaliss
force-pushed
the
harmonize-suitenumerique-os
branch
from
5882f18 to
d0230e3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Move helm chart, prod compose examples and buildpack scripts under deploy/{kubernetes,docker,paas}/. Wire THEME_CUSTOMIZATION_JSON directly in settings so the env var works on Docker and k8s without the Scalingo buildpack pivot to a file.
Purpose
Harmonize this repo's deployments with other repositories from suitenumerique
External contributions
Thank you for your contribution! 🎉
Please ensure the following items are checked before submitting your pull request:
General requirements
Skip the checkbox below 👇 if you're fixing an issue or adding documentation
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution