Skip to content

feat: add log rotation env vars#563

Closed
CamronStaley wants to merge 1 commit into
mainfrom
feat/do-log-cron
Closed

feat: add log rotation env vars#563
CamronStaley wants to merge 1 commit into
mainfrom
feat/do-log-cron

Conversation

@CamronStaley

@CamronStaley CamronStaley commented May 11, 2026

Copy link
Copy Markdown
Contributor

Rationale

Logs are now stored in mongo for 30 days and then automatically rotated to a cloud path or deleted. If the path isn't provided they're just deleted. This PR exposes those values and documents them for customers.

These PRs need merged before these can be:

Review Priority

  • high
  • medium
  • low

Changes

Document FIFTYONE_DO_LOG_BUCKET_PATH and FIFTYONE_DO_LOG_RETENTION_DAYS with a default of "None" and "30 days"

Checklist

  • This PR maintains parity between Docker Compose and Helm

Testing

unit tests

Related

@CamronStaley CamronStaley self-assigned this May 11, 2026
@coderabbitai

coderabbitai Bot commented May 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

Two new environment variables for delegated operation log handling are documented in Docker and Helm READMEs, with corresponding test case updates across six TestContainerEnv scenarios to validate expected container environment variable rendering.

Changes

Delegated operation log environment variables

Layer / File(s) Summary
Documentation of log retention and upload configuration
docker/README.md, helm/fiftyone-teams-app/README.md
Docker and Helm README files document FIFTYONE_DO_LOG_BUCKET_PATH (optional cloud bucket prefix for log uploads) and FIFTYONE_DO_LOG_RETENTION_DAYS (MongoDB retention period in days).
Test expectations for log configuration variables
tests/unit/helm/api-deployment_test.go
TestContainerEnv test expectations updated across six scenarios (defaultValues, overrideEnv, internalAuthMode, overrideSecretName, overrideCasServiceNameAndPort, overrideExternalApiUrl) to include the new environment variables in rendered container configuration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 Two vars hop into the logs today,
One guards the bucket where they'll stay,
The other counts the retention days—
Documentation and tests light up the ways! 📝✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat: add log rotation env vars' is concise and directly summarizes the main change—documenting two new environment variables for log rotation behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The PR description follows the required template structure with all major sections completed: Rationale, Changes with checklist, Testing, and Related links.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/do-log-cron

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Comment @coderabbitai help to get the list of available commands and usage tips.

@CamronStaley CamronStaley marked this pull request as ready for review May 12, 2026 21:54
@CamronStaley CamronStaley requested a review from a team as a code owner May 12, 2026 21:54

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/unit/helm/api-deployment_test.go (1)

425-1206: ⚡ Quick win

Add one explicit override case for the new log env vars.

Current updates verify defaults in many scenarios, but not user-provided overrides. A dedicated case will catch template wiring regressions for these keys.

Proposed test-case addition
@@ func (s *deploymentApiTemplateTest) TestContainerEnv() {
 		{
+			"overrideDoLogEnvVars",
+			map[string]string{
+				"apiSettings.env.FIFTYONE_DO_LOG_BUCKET_PATH":    "s3://bucket/prefix",
+				"apiSettings.env.FIFTYONE_DO_LOG_RETENTION_DAYS": "14",
+			},
+			func(envVars []corev1.EnvVar) {
+				require.Contains(s.T(), envVars, corev1.EnvVar{
+					Name:  "FIFTYONE_DO_LOG_BUCKET_PATH",
+					Value: "s3://bucket/prefix",
+				})
+				require.Contains(s.T(), envVars, corev1.EnvVar{
+					Name:  "FIFTYONE_DO_LOG_RETENTION_DAYS",
+					Value: "14",
+				})
+			},
+		},
+		{
 			"overrideExternalApiUrl",
 			map[string]string{
🤖 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 `@tests/unit/helm/api-deployment_test.go` around lines 425 - 1206, Add a new
test case inside TestContainerEnv that explicitly verifies user-provided
overrides for the logging env vars (e.g. override "LOGGING_LEVEL" and
"FIFTYONE_LOGGING_FORMAT"). Create a case (name like "overrideLogEnvVars") that
sets the corresponding values in the values map (use the same prefix pattern as
other overrides, e.g. "apiSettings.env.LOGGING_LEVEL": "DEBUG" and
"apiSettings.env.FIFTYONE_LOGGING_FORMAT": "json"), then assert that the
resulting envVars slice contains the updated values for LOGGING_LEVEL and
FIFTYONE_LOGGING_FORMAT (match the JSON-based expectedEnvVar JSON pattern used
by other cases in TestContainerEnv).
🤖 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.

Nitpick comments:
In `@tests/unit/helm/api-deployment_test.go`:
- Around line 425-1206: Add a new test case inside TestContainerEnv that
explicitly verifies user-provided overrides for the logging env vars (e.g.
override "LOGGING_LEVEL" and "FIFTYONE_LOGGING_FORMAT"). Create a case (name
like "overrideLogEnvVars") that sets the corresponding values in the values map
(use the same prefix pattern as other overrides, e.g.
"apiSettings.env.LOGGING_LEVEL": "DEBUG" and
"apiSettings.env.FIFTYONE_LOGGING_FORMAT": "json"), then assert that the
resulting envVars slice contains the updated values for LOGGING_LEVEL and
FIFTYONE_LOGGING_FORMAT (match the JSON-based expectedEnvVar JSON pattern used
by other cases in TestContainerEnv).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1b582b33-90f3-40f2-aace-e672acc7cf8c

📥 Commits

Reviewing files that changed from the base of the PR and between 816edea and c7e96f1.

⛔ Files ignored due to path filters (2)
  • helm/fiftyone-teams-app/values.schema.json is excluded by !**/*.json
  • helm/fiftyone-teams-app/values.yaml is excluded by !**/*.yaml
📒 Files selected for processing (3)
  • docker/README.md
  • helm/fiftyone-teams-app/README.md
  • tests/unit/helm/api-deployment_test.go

@kevin-dimichel

Copy link
Copy Markdown
Contributor

@CamronStaley - is this intended to to be released with the next minor FiftyOne Enterprise release?

@CamronStaley

Copy link
Copy Markdown
Contributor Author

@kevin-dimichel yes the changes that use these env vars are on develop and will go out in the release branch

@kevin-dimichel kevin-dimichel added the priority-low Low Priority PR - review today label May 13, 2026
@CamronStaley

Copy link
Copy Markdown
Contributor Author

These were moved to OSS / FOE docs as discussed offline. If customers struggle to fine them then we will add back here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority-low Low Priority PR - review today

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants