Skip to content

Conversation

@hhzhang16
Copy link
Contributor

@hhzhang16 hhzhang16 commented Jan 27, 2026

Overview:

Use existing consts instead of relying on hardcoded strings for statuses within the Dynamo operator. Also, differentiate DGDR vs DGD statuses.

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Refactor
    • Updated internal state management constants to improve code organization and consistency across deployment controllers.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Hannah Zhang <hannahz@nvidia.com>
@hhzhang16 hhzhang16 requested a review from a team as a code owner January 27, 2026 21:46
@github-actions github-actions bot added feat deployment::k8s Relates to dynamo deployment in kubernetes labels Jan 27, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 27, 2026

Walkthrough

Two controller files undergo systematic state constant renaming to improve namespace clarity: dynamographdeployment_controller.go adds DGDState prefix to three constants, while dynamographdeploymentrequest_controller.go renames seven state constants with DGDRState prefix. All references throughout the reconciliation logic are updated consistently.

Changes

Cohort / File(s) Summary
State Constant Refactoring
deploy/operator/internal/controller/dynamographdeployment_controller.go
Renamed public state constants: FailedStateDGDStateFailed, ReadyStateDGDStateReady, PendingStateDGDStatePending. Updated all state transitions and status comparisons in reconciliation flow to use new constant names.
State Constant Refactoring
deploy/operator/internal/controller/dynamographdeploymentrequest_controller.go
Replaced seven state constants with DGDRState* prefix variants: StateEmpty, StatePending, StateProfiling, StateDeploying, StateReady, StateDeploymentDeleted, StateFailed. Updated state machine logic, immutability checks, state transitions, and all status comparisons throughout the controller.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Constants renamed with style so bright,
DGDState and DGDRState set things right,
No logic changed, just clarity gained,
These prefixed names have been ordained,
State machines hop along in delight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The description provides an overview and mentions the main objectives, but the Details and Where should the reviewer start sections are empty. Add specific descriptions of changes made and call out key files (deploy/operator/internal/controller/dynamographdeployment_controller.go and dynamographdeploymentrequest_controller.go) for reviewer focus.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: replacing hardcoded strings with constants for statuses in the Dynamo operator.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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


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.

❤️ Share

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
deploy/operator/internal/controller/dynamographdeploymentrequest_controller.go (1)

665-686: Replace the remaining hardcoded "Pending" to keep state casing consistent.
This PR’s goal is to remove hardcoded status strings; the success path still sets "Pending" which now mismatches DGDStatePending ("pending").

🔧 Suggested fix
-    State:     "Pending",
+    State:     string(DGDStatePending),

Copy link
Contributor

@julienmancuso julienmancuso left a comment

Choose a reason for hiding this comment

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

Actually, coderabbit made a good comment, please take it into account

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

Labels

deployment::k8s Relates to dynamo deployment in kubernetes feat size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants