Add pointer docs for access-provisioning and deployment - #997
Conversation
Short, repo-visible pointers to the fuller runbooks now written up in the team's Obsidian vault: - docs/ACCESS.md: how Django admin accounts and production/staging SSH access get requested and granted (personal credentials only, least privilege, logged grants). - docs/DEPLOYMENT.md: the Docker Compose deploy workflow and known rough edges (misleading prod hostname, missing log rotation, the in-progress Kubernetes migration). No code changes. Written while investigating #996, which surfaced the need for a documented access-request process.
|
Caution Review failedFailed to post review comments. We encountered an issue with GitHub. Use ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout. (1)
🧰 Additional context used🪛 LanguageTooldocs/ACCESS.md[grammar] ~13-~13: A determiner may be missing. (THE_SUPERLATIVE) docs/DEPLOYMENT.md[uncategorized] ~19-~19: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) [style] ~39-~39: To elevate your writing, try using a synonym like ‘required’ here. Or, to avoid using the passive voice, try replacing the past participle ‘needed’ with an adjective. (IS_NEEDED_NECESSARY) [grammar] ~41-~41: The verb ‘Compose’ is plural. Did you mean: “composes”? Did you use a verb instead of a noun? (PLURAL_VERB_AFTER_THIS) 📝 WalkthroughWalkthroughAdded access and credential provisioning guidance. Added staging and production deployment procedures, container checks, and operational warnings. ChangesAccess and deployment documentation
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 7
🧹 Nitpick comments (1)
docs/DEPLOYMENT.md (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the migration warning to its authoritative plan.
“Per recent commit history” is not reproducible for operators and will become stale. Link to
../k8s/MIGRATION.mdand add a last-reviewed date or owner.k8s/MIGRATION.md, Lines 3-8, already documents the Kubernetes prerequisites.🤖 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/DEPLOYMENT.md` around lines 40 - 42, Update the Kubernetes migration warning near the Docker Compose workflow to link directly to ../k8s/MIGRATION.md instead of referencing recent commit history, and add a last-reviewed date or responsible owner alongside the warning.
🤖 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 `@docs/ACCESS.md`:
- Around line 20-24: Update the “Shared secrets” documentation in ACCESS.md to
include DJANGO_SECRET_KEY alongside POSTGRES_PASSWORD and RABBIT_PASSWORD,
covering both .env and Kubernetes Secret storage. State that the same access
restrictions, non-disclosure guidance, and rotation-on-departure rules apply to
all deployment secrets.
In `@docs/DEPLOYMENT.md`:
- Around line 15-16: Update the deployment instructions around the “sudo -i &&
cd /cantus-ultimus” step to avoid granting a persistent root shell. Replace it
with command-scoped sudo usage for the required git and Docker operations, or
explicitly restrict the procedure to sysadmins while preserving the repository
ownership requirements and alignment with docs/ACCESS.md.
- Around line 15-16: Update the deployment instructions around the sudo setup
step: do not chain `sudo -i` with `cd`; instruct users to change to
/cantus-ultimus within the elevated root shell, then verify the context with id
-u and pwd before proceeding.
- Around line 25-26: Update the verification step in DEPLOYMENT.md to validate
service health in addition to docker container state, using Compose health
checks or an application smoke test. Document which exited one-shot containers
are expected, and reference the existing Kubernetes readiness-probe behavior
defined by the deployment configuration.
- Around line 11-14: Update the deployment instructions around the SSH
verification step to require an independent production-host check, such as
comparing the resolved host address or a documented production marker, before
any privileged commands; do not rely on hostname alone. Apply the same
verification requirement to the related instructions around the later
privileged-command step.
- Around line 19-21: Update the deployment steps around git fetch/pull and
submodule validation to fetch and check out the approved commit or release tag
instead of pulling the latest main. Treat -, +, and U submodule prefixes as stop
conditions, require no unresolved paths after restoring the stash, and only then
proceed to restart services.
- Around line 22-24: Update the backend restart guidance in the deployment
instructions to rebuild and recreate the app and celery services with `docker
compose up -d --build app celery` before running them. Do not recommend `docker
compose restart` alone for backend-only changes; retain the existing frontend
rebuild guidance.
---
Nitpick comments:
In `@docs/DEPLOYMENT.md`:
- Around line 40-42: Update the Kubernetes migration warning near the Docker
Compose workflow to link directly to ../k8s/MIGRATION.md instead of referencing
recent commit history, and add a last-reviewed date or responsible owner
alongside the warning.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96a0ea1d-0f72-4452-9fd6-04ca6a2a7dc6
📒 Files selected for processing (2)
docs/ACCESS.mddocs/DEPLOYMENT.md
| - **Shared secrets** (`.env`: `POSTGRES_PASSWORD`, `RABBIT_PASSWORD`): these are | ||
| infrastructure secrets, not per-person credentials. Only the sysadmin and | ||
| active maintainers should hold them; never commit them or paste them into | ||
| chat, issues, or docs. Rotate them if someone who held them leaves the | ||
| project. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document every deployment secret.
docker-compose.yaml, Lines 19-28, passes DJANGO_SECRET_KEY to the application, but this list names only POSTGRES_PASSWORD and RABBIT_PASSWORD. The staging deployment also stores DJANGO_SECRET_KEY in a Kubernetes Secret (k8s/cantus-ultimus-staging/app/deployment.yaml, Lines 16-49). Include this key and define the same protection and rotation rules for both .env values and Kubernetes Secret values.
🤖 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/ACCESS.md` around lines 20 - 24, Update the “Shared secrets”
documentation in ACCESS.md to include DJANGO_SECRET_KEY alongside
POSTGRES_PASSWORD and RABBIT_PASSWORD, covering both .env and Kubernetes Secret
storage. State that the same access restrictions, non-disclosure guidance, and
rotation-on-departure rules apply to all deployment secrets.
| 1. `ssh` into the target host using your personal SSH config alias, then | ||
| confirm you're on the right box with `hostname` — **the production | ||
| hostname is misleadingly named and does not look like a production | ||
| server; do not assume it's safe to experiment on because of its name.** |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use an independent production-host check.
hostname does not confirm the target because the document states that the production hostname is misleading. Require the operator to compare an expected host address or documented production marker before running privileged commands.
Also applies to: 30-32
🤖 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/DEPLOYMENT.md` around lines 11 - 14, Update the deployment instructions
around the SSH verification step to require an independent production-host
check, such as comparing the resolved host address or a documented production
marker, before any privileged commands; do not rely on hostname alone. Apply the
same verification requirement to the related instructions around the later
privileged-command step.
| 2. `sudo -i && cd /cantus-ultimus` (the repo there is owned by `root`; all | ||
| git/docker operations must run as root). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep deployment privilege scoped to commands.
sudo -i grants a full root shell to any operator who can follow this runbook. This conflicts with the least-privilege guidance in docs/ACCESS.md, Lines 15-24, and exposes shared .env secrets. Use command-scoped privileges, or state clearly that only sysadmins may perform this step.
🤖 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/DEPLOYMENT.md` around lines 15 - 16, Update the deployment instructions
around the “sudo -i && cd /cantus-ultimus” step to avoid granting a persistent
root shell. Replace it with command-scoped sudo usage for the required git and
Docker operations, or explicitly restrict the procedure to sysadmins while
preserving the repository ownership requirements and alignment with
docs/ACCESS.md.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Do not chain sudo -i with cd.
sudo -i opens a separate interactive shell and waits until that shell exits. The cd /cantus-ultimus command then runs in the original shell. The root shell remains in its login directory. Run cd /cantus-ultimus inside the elevated shell, then verify id -u and pwd before continuing.
🤖 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/DEPLOYMENT.md` around lines 15 - 16, Update the deployment instructions
around the sudo setup step: do not chain `sudo -i` with `cd`; instruct users to
change to /cantus-ultimus within the elevated root shell, then verify the
context with id -u and pwd before proceeding.
| 4. `git fetch && git pull`, then `git submodule status` — flag any `-` or `+` | ||
| prefixed submodule to the sysadmin before continuing. | ||
| 5. `git stash pop` to restore local modifications; resolve conflicts if any. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Pin production deployments to an approved revision.
git pull can deploy a newer main commit than the one reviewed for release. Fetch and check out an approved commit or release tag. Treat a U submodule prefix as a stop condition in addition to - and +, and require no unresolved paths before restarting services.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~19-~19: Loose punctuation mark.
Context: ...m if present. 4. git fetch && git pull, then git submodule status — flag any ...
(UNLIKELY_OPENING_PUNCTUATION)
🤖 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/DEPLOYMENT.md` around lines 19 - 21, Update the deployment steps around
git fetch/pull and submodule validation to fetch and check out the approved
commit or release tag instead of pulling the latest main. Treat -, +, and U
submodule prefixes as stop conditions, require no unresolved paths after
restoring the stash, and only then proceed to restart services.
| 6. Restart: `docker compose restart` for backend-only changes. For frontend | ||
| changes (`cantus-min.js` / Diva.js), rebuild first: | ||
| `docker compose build nginx && docker compose up -d nginx`. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether services use images, builds, source volumes, or health checks.
rg -n -C 8 '(^services:|^[[:space:]]+[A-Za-z0-9_.-]+:|^[[:space:]]+(build|image|volumes|healthcheck):)' docker-compose.yamlRepository: DDMAL/cantus
Length of output: 3320
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- app Dockerfile ---'
fd -i 'Dockerfile' . | sort
app_dockerfile=$(fd -i 'Dockerfile' . | awk '$0 ~ /(^|\/)app\/Dockerfile$/ {print; exit}')
if [ -n "${app_dockerfile}" ]; then
cat -n "${app_dockerfile}"
fi
printf '%s\n' '--- deployment instructions ---'
cat -n docs/DEPLOYMENT.md | sed -n '1,35p'
printf '%s\n' '--- backend source mounts and image references ---'
rg -n -C 3 '(/code|app/|cantus-app|docker compose (build|up|restart))' docker-compose.yaml "$app_dockerfile" docs/DEPLOYMENT.mdRepository: DDMAL/cantus
Length of output: 8101
Rebuild the backend image before restarting the services.
The app and celery services use the cantus-app image and do not mount backend source files. Backend changes require docker compose up -d --build app celery; docker compose restart alone runs stale code.
🤖 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/DEPLOYMENT.md` around lines 22 - 24, Update the backend restart guidance
in the deployment instructions to rebuild and recreate the app and celery
services with `docker compose up -d --build app celery` before running them. Do
not recommend `docker compose restart` alone for backend-only changes; retain
the existing frontend rebuild guidance.
| 7. Verify: `docker ps -a` — all containers should be `Up`. If any are | ||
| `Exited`, check `docker logs <container-name>`. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Check service health, not only container state.
docker ps -a can show a container as Up while the application or a dependency is unhealthy. Checking logs only for Exited can miss failed readiness checks. Add Compose health checks or an application smoke test, and document which exited one-shot containers are expected. The Kubernetes deployment already defines a readiness probe in k8s/cantus-ultimus-staging/app/deployment.yaml, Lines 16-49.
🤖 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/DEPLOYMENT.md` around lines 25 - 26, Update the verification step in
DEPLOYMENT.md to validate service health in addition to docker container state,
using Compose health checks or an application smoke test. Document which exited
one-shot containers are expected, and reference the existing Kubernetes
readiness-probe behavior defined by the deployment configuration.
1 participant
Short, repo-visible pointers to the fuller runbooks now written up in the team's Obsidian vault:
docs/ACCESS.md: how Django admin accounts and production/staging SSH access get requested and granted (personal credentials only, least privilege, logged grants).docs/DEPLOYMENT.md: the Docker Compose deploy workflow and known rough edges (misleading prod hostname, missing log rotation, the in-progress Kubernetes migration).No code changes. Written while investigating #996, which surfaced the need for a documented access-request process.
🤖 Generated with Claude Code
Summary by CodeRabbit