feat: Add legacy DO expiration #492
Conversation
WalkthroughEnvironment variable configuration for delegated operation expiration handling has been refactored. The naming scheme shifts from generic duration concepts to explicit monitored and legacy (unmonitored) expiration controls, with corresponding updates to documentation and test assertions. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
This reverts commit 63a6cfc.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/helm/api-deployment_test.go (1)
489-496: CRITICAL: Inconsistent environment variable names across test cases.Only the
overrideSecretNametest case (lines 806-811) was updated with the new variable naming scheme (FIFTYONE_DO_EXPIRATION_MINUTESwith value "30" andFIFTYONE_DO_LEGACY_EXPIRATION_MINUTESwith empty value), but all other test cases still reference the old variable names (FIFTYONE_DO_EXPIRATION_DAYSand the oldFIFTYONE_DO_EXPIRATION_MINUTES).This inconsistency will cause tests to fail or validate incorrect behavior. All test cases in
TestContainerEnvshould be updated to use the new variable names and values to match the actual Helm chart behavior.Apply this diff to update the remaining test cases:
{ "defaultValues", // legacy auth mode nil, func(envVars []corev1.EnvVar) { expectedEnvVarJSON := `[ ... { - "name": "FIFTYONE_DO_EXPIRATION_DAYS", - "value": "1" + "name": "FIFTYONE_DO_EXPIRATION_MINUTES", + "value": "30" }, { - "name": "FIFTYONE_DO_EXPIRATION_MINUTES", + "name": "FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES", "value": "" },Repeat this pattern for:
overrideEnvtest case (lines 587-593)internalAuthModetest case (lines 698-704)overrideCasServiceNameAndPorttest case (lines 902-908)overrideExternalApiUrltest case (lines 998-1004)Also applies to: 587-593, 698-704, 902-908, 998-1004
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker/README.md(1 hunks)tests/unit/helm/api-deployment_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: helm-unit / unit-helm
- GitHub Check: helm-integration / integration-helm
- GitHub Check: docker-pulls
- GitHub Check: pre-commit
🔇 Additional comments (2)
tests/unit/helm/api-deployment_test.go (1)
806-811: LGTM: Correct variable renaming in this test case.The environment variable updates in the
overrideSecretNametest case correctly reflect the new naming scheme:
FIFTYONE_DO_EXPIRATION_MINUTESnow represents monitored delegated operation timeout (30 minutes)FIFTYONE_DO_LEGACY_EXPIRATION_MINUTESrepresents unmonitored delegated operation timeoutHowever, ensure this same update is applied to all other test cases (see previous comment).
docker/README.md (1)
842-842: LGTM: Clear documentation for monitored delegated operations.The updated description for
FIFTYONE_DO_EXPIRATION_MINUTESclearly explains its purpose for monitored delegated operations and specifies the 30-minute default timeout. The distinction between monitored and unmonitored (legacy) operations is helpful.
| | `FIFTYONE_DO_EXPIRATION_DAYS` | The amount of time in days that a delegated operation can run before being automatically terminated. Default is 1 day. | No | | ||
| | `FIFTYONE_DO_EXPIRATION_MINUTES` | The amount of time in minutes that a delegated operation can run before being automatically terminated. Default is `FIFTYONE_DO_EXPIRATION_DAYS` converted to minutes. If this field is provided it will override `FIFTYONE_DO_EXPIRATION_DAYS`. | No | | ||
| | `FIFTYONE_DO_EXPIRATION_MINUTES` | The amount of time in minutes that a monitored delegated operation can run without reporting its status before being automatically terminated. Default is 30 minutes. | No | | ||
| | `FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES` | The amount of time in minutes that an unmonitored delegated operation can run before being automatically terminated. Default is `FIFTYONE_DO_EXPIRATION_DAYS` converted to minutes. If this field is provided it will override `FIFTYONE_DO_EXPIRATION_DAYS`. | No | |
There was a problem hiding this comment.
Clarify the reference to undocumented FIFTYONE_DO_EXPIRATION_DAYS variable.
Line 843 references FIFTYONE_DO_EXPIRATION_DAYS in the description of FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES, stating it can override that variable and is used as the default. However, FIFTYONE_DO_EXPIRATION_DAYS does not appear in the environment variables table, and the PR objectives indicate this variable should be removed from documentation.
Please clarify:
- If
FIFTYONE_DO_EXPIRATION_DAYSis deprecated but still supported for backward compatibility, add a note explaining this and consider documenting it with a deprecation warning. - If
FIFTYONE_DO_EXPIRATION_DAYSis fully removed, update the description to remove references to it.
Consider this revision for line 843:
-| `FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES` | The amount of time in minutes that an unmonitored delegated operation can run before being automatically terminated. Default is `FIFTYONE_DO_EXPIRATION_DAYS` converted to minutes. If this field is provided it will override `FIFTYONE_DO_EXPIRATION_DAYS`. | No |
+| `FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES` | The amount of time in minutes that an unmonitored delegated operation can run before being automatically terminated. If not set, defaults to 1440 minutes (24 hours). | No |🤖 Prompt for AI Agents
In docker/README.md around line 843, the description for
FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES references an undocumented variable
FIFTYONE_DO_EXPIRATION_DAYS; update the docs to remove ambiguity by either (A)
if the days variable is deprecated-but-still-supported: add a one-line
deprecation note and document FIFTYONE_DO_EXPIRATION_DAYS with its default
behavior and warning that FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES overrides it, or
(B) if the days variable is fully removed: delete the reference to
FIFTYONE_DO_EXPIRATION_DAYS and state the default explicitly (e.g., provide the
minute default or say it must be set), ensuring the description clearly states
that FIFTYONE_DO_LEGACY_EXPIRATION_MINUTES overrides any legacy setting.
There was a problem hiding this comment.
curious folk's thoughts on this. The reason I removed it is because we are only continuing to default to it for backwards compatibility in legacy orchestrators and the naming is confusing since it doesn't contain "legacy" like minutes does. I am open to adding it back with a "DEPRECATED" tag of some sort if people think that's the better approach.
There was a problem hiding this comment.
idk if we have a way of deprecating things like that. You could add a small note to the upgrade guide for 2.XX?
There was a problem hiding this comment.
We can consider keeping FIFTYONE_DO_EXPIRATION_DAYS documented in the table and in the description include a deprecated reference. We can also mention that the flag will be removed in X releases (at least 2).
Stuart makes a good point about including a note in the upgrade guide(s). We can instruct the user to replaced the deprecated with its replacement.
There was a problem hiding this comment.
Okay updated lmk yalls thoughts
kevin-dimichel
left a comment
kevin-dimichel
left a comment
There was a problem hiding this comment.
Is this PR intended to be merged upon the next release? The linked PRs were merged after the latest release.
|
@kevin-dimichel yes the intention is to get this merged with the next release of develop. |
kevin-dimichel
changed the title
Rationale
Parity with these changes: https://github.com/voxel51/voxel-hub/pull/2009
Changes
Remove documentation for the old "days" environment variable and replace it with the new "legacy" env var.
Checklist
Testing
unit tests
Notes
This feature will be released in the next release. Once the release branch is made, we will update the target branch.