Add ExporterStatus enum and status field #26
Conversation
WalkthroughAdds an output-only Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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 |
|
@mangelajo Will this break anything if we merge, or is there a specific order of steps that we need to go through on the controller side? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proto/jumpstarter/client/v1/client.proto (2)
71-72: Makestatusoptional for presence semantics; define relationship withonline.
- Consider
optionalso clients can distinguish "unset" vs "UNSPECIFIED", consistent with other fields in this proto (e.g., optional timestamps).- Clarify/standardize how
onlinerelates tostatusto avoid divergence. A simple rule would beonline == (status != EXPORTER_STATUS_OFFLINE). If that’s the intent, plan to deprecateonline(later) or mark it as derived.Suggested change in this hunk:
- ExporterStatus status = 4 [(google.api.field_behavior) = OUTPUT_ONLY]; + optional ExporterStatus status = 4 [(google.api.field_behavior) = OUTPUT_ONLY];Outside this hunk (for later, if you choose to deprecate):
bool online = 3 [(google.api.field_behavior) = OUTPUT_ONLY, deprecated = true];Please confirm whether
onlineshould be strictly derived fromstatusand whether servers will always populatestatus(making presence unnecessary).
74-81: Enum looks good; consider minor naming/docs polish.
- Zero value UNSPECIFIED is correct. Good.
- Optional: align with common API style by naming this “State” (ExporterState) rather than “Status” to convey lifecycle; not required.
- Add brief comments per value to improve generated docs, since these are user-facing states.
Example:
// The exporter is reachable and can accept a lease. EXPORTER_STATUS_AVAILABLE = 2;
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proto/jumpstarter/client/v1/client.proto (1)
74-81: Document state machine expectations and availability semantics.Enum design looks solid. Add a brief enum-level comment clarifying transitions and availability so clients can reason safely (e.g., only
AVAILABLEindicates free-to-lease; other states are not available). This improves forward compatibility.Apply this diff to add a synopsis:
-enum ExporterStatus { +// Describes the exporter lifecycle/status for client consumption. +// Notes: +// - Only EXPORTER_STATUS_AVAILABLE indicates the exporter can be leased. +// - Additional states may be added in the future; clients should default +// to treating unknown states as not available. +// - Mapping to legacy `online`: online == (status != OFFLINE). +enum ExporterStatus { EXPORTER_STATUS_UNSPECIFIED = 0; // Unspecified exporter status EXPORTER_STATUS_OFFLINE = 1; // Exporter is offline EXPORTER_STATUS_AVAILABLE = 2; // Exporter is available to be leased EXPORTER_STATUS_BEFORE_LEASE_HOOK = 3; // Exporter is executing before lease hook(s) EXPORTER_STATUS_LEASE_READY = 4; // Exporter is leased and ready to accept commands EXPORTER_STATUS_AFTER_LEASE_HOOK = 5; // Exporter is executing after lease hook(s) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proto/jumpstarter/client/v1/client.proto(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-06T11:35:36.215Z
Learnt from: michalskrivanek
PR: jumpstarter-dev/jumpstarter-protocol#24
File: proto/jumpstarter/client/v1/client.proto:70-70
Timestamp: 2025-08-06T11:35:36.215Z
Learning: In the jumpstarter-protocol repository, the user michalskrivanek intentionally uses non-optional boolean fields for status indicators like the `online` field in the Exporter message, preferring to treat unset values as false rather than distinguishing between unset and explicitly false states.
Applied to files:
proto/jumpstarter/client/v1/client.proto
🔇 Additional comments (1)
proto/jumpstarter/client/v1/client.proto (1)
70-72: Document mapping fromonline→statusand treatEXPORTER_STATUS_UNSPECIFIEDas unknown.proto/jumpstarter/client/v1/client.proto already contains deprecated
online(field 3) andstatus(field 4, enum includes EXPORTER_STATUS_UNSPECIFIED). Add these comments to guide migration:string name = 1 [(google.api.field_behavior) = IDENTIFIER]; map<string, string> labels = 2; - bool online = 3 [(google.api.field_behavior) = OUTPUT_ONLY, deprecated = true]; - ExporterStatus status = 4 [(google.api.field_behavior) = OUTPUT_ONLY]; + // Deprecated: use `status` instead. + // Compatibility: servers should continue to populate this for a grace period. + // Mapping: treat `online == true` iff `status != EXPORTER_STATUS_OFFLINE`. + bool online = 3 [(google.api.field_behavior) = OUTPUT_ONLY, deprecated = true]; + // Overall exporter lifecycle status. + // Default/zero value is `EXPORTER_STATUS_UNSPECIFIED`; clients should treat that + // as unknown (and not assume availability). + ExporterStatus status = 4 [(google.api.field_behavior) = OUTPUT_ONLY]; }Operational sequence:
- Merge proto and regenerate SDKs.
- Update controller to populate
statuswhile continuing to setonlineper the mapping.- After clients migrate, remove
onlineand reserve field number 3.Confirm the controller will emit both fields during the migration window and that client logic treats
EXPORTER_STATUS_UNSPECIFIEDas unknown.
Add exporter status enum as proposed in #25
Summary by CodeRabbit
New Features
Chores