Sid/edit docker docs#565
Conversation
- Add Step 0 for MongoDB setup (Atlas, Community, Enterprise) - Merge auth mode selection into Step 2, defaulting to internal-auth - Default docker command to dedicated-plugins + delegated-operators - Remove plugin modes section from main steps; move advanced DO config (GPU, custom images) to Advanced Configuration - Simplify Step 4 to path-based routing only - Add Step 5 for DO log path configuration in compose.override.yaml - Mark IdP/CAS step as optional for airgapped deployments - Fix FIFTYONE_DELEGATED_OPERATION_LOG_PATH (renamed from RUN_LINK_PATH in v2.7.0) - Update custom plugin image base to voxel51/fiftyone-teams-cv-full Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove "recommended" label from Atlas to avoid opinionated guidance - Fix step reference from Step 3 to Step 2 after renumbering Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move GPU, custom plugin images, and on-demand DO executors out of Advanced Configuration into a new top-level Advanced DO Settings section - Rewrite Upgrades with recommended git pull + down/up workflow, notes on FIFTYONE_DATABASE_ADMIN and custom image version bumps Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…build Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR updates the Docker deployment documentation with restructured step numbering and guidance. It adds foundational MongoDB setup instructions, reorganizes environment configuration, clarifies that Dedicated Plugins and Delegated Operators are enabled by default, simplifies Nginx configuration, introduces delegated operation logging configuration, and replaces upgrade guidance with new advanced DO settings and a refreshed upgrade procedure. ChangesDeployment Guide Restructuring
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/README.md (1)
251-263:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the
FIFTYONE_DATABASE_ADMINcontradiction in initial setup.Line 251 says to enable DB admin mode, but Line 259 sets it to
false, and Line 262 says it allows schema creation/migration. This is internally inconsistent and can break first-time install guidance.Suggested doc fix
### 1. Enable Database Admin mode In `compose.override.yaml`, make sure: ```yaml services: fiftyone-app: environment: - FIFTYONE_DATABASE_ADMIN: false + FIFTYONE_DATABASE_ADMIN: trueThen keep `false` in the upgrade section where you already correctly require it. </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docker/README.mdaround lines 251 - 263, The README's initial setup
contradicts itself by telling users to "Enable Database Admin mode" but showing
FIFTYONE_DATABASE_ADMIN: false; update the compose.override.yaml example under
"1. Enable Database Admin mode" to set FIFTYONE_DATABASE_ADMIN: true so the app
can create/migrate the schema, and leave the FIFTYONE_DATABASE_ADMIN: false
value only in the upgrade section as currently documented; specifically edit the
environment value for FIFTYONE_DATABASE_ADMIN in the compose.override.yaml
example referenced in this section.</details> </blockquote></details> </blockquote></details>🤖 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 `@docker/README.md`: - Around line 407-408: Replace the incorrect noun "setup" with the verb phrase "set up" in the sentence that currently reads "To setup containers with GPU resources, see the [configuring GPU workloads documentation](./docs/configuring-gpu-workloads.md)." — update that string to "To set up containers with GPU resources, see the [configuring GPU workloads documentation](./docs/configuring-gpu-workloads.md)." ensuring only the wording is changed and the link remains intact. - Around line 349-358: The docs currently use two different environment variable names—FIFTYONE_DELEGATED_OPERATION_LOG_PATH (in Step 5) and FIFTYONE_DELEGATED_OPERATION_RUN_LINK_PATH (in the Environment Variables table)—which will confuse users; unify them by choosing the intended variable name (e.g., FIFTYONE_DELEGATED_OPERATION_LOG_PATH) and update every occurrence: change the Environment Variables table entry for FIFTYONE_DELEGATED_OPERATION_RUN_LINK_PATH to the chosen name, update any examples or references that mention the alternate symbol, and ensure the description/documentation of FIFTYONE_DELEGATED_OPERATION_LOG_PATH matches the usage shown in Step 5. --- Outside diff comments: In `@docker/README.md`: - Around line 251-263: The README's initial setup contradicts itself by telling users to "Enable Database Admin mode" but showing FIFTYONE_DATABASE_ADMIN: false; update the compose.override.yaml example under "1. Enable Database Admin mode" to set FIFTYONE_DATABASE_ADMIN: true so the app can create/migrate the schema, and leave the FIFTYONE_DATABASE_ADMIN: false value only in the upgrade section as currently documented; specifically edit the environment value for FIFTYONE_DATABASE_ADMIN in the compose.override.yaml example referenced in this section.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
| ## Step 8: Test End User Login | ||
|
|
||
| ### :hammer_and_wrench: What Happens If You Migrate with database admin False? | ||
| Verify the deployment is working end-to-end by logging in as a regular user. |
There was a problem hiding this comment.
Yes. But let's not call it an end-to-end when it isn't : D
Please update.
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
- Complete step renumbering (Steps 1-10) and fix all ToC anchors - Add Step 3: Choose Authentication Mode (internal-auth for OIDC/air-gapped, legacy-auth for SAML) - Remove Air Gapped Deployments Only label from IdP/CAS step - Split Upgrades bring-down and bring-up into separate list items - Add MongoDB 8.0+ recommendation - Fix in-text step cross-references - Add compose.override.yaml example for custom plugin images Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
kevin-dimichel
left a comment
There was a problem hiding this comment.
Almost there! After these, we can approve.
Co-authored-by: Kevin DiMichel <56850465+kevin-dimichel@users.noreply.github.com> Signed-off-by: Sid Mehta <siddharthm2350@gmail.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rationale
Docs change to improve the end-user experience for customers who want to install docker via FOE
Review Priority
Changes
Checklist
Testing