status: separate internal state from public status - #673
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed nil-pointer panic paths in pkg/types/fingerprint.go, and the kagent Remove flow likely regresses teardown correctness when runtime namespace configuration changes because namespace is no longer persisted in internal meta.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors Deployment/Runtime persistence so that adapter/controller internal state is stored separately from the public status API. It introduces a dedicated internal_meta storage column and typed status patch helpers, and removes the previously-opaque status.details bag from the v1alpha1 API surface.
Changes:
- Add
internal_metaJSONB column for Deployments and Runtimes, plus store support to patch it independently or atomically with status. - Introduce
types.DeploymentRecord/DeploymentInternalMetaand thread it through adapter/controller flows so internal state no longer lives in public status. - Replace generic
Statususage in Deployment/Runtime status with typedDeploymentStatus/RuntimeStatusand add typed patcher helpers.
File summaries
| File | Description |
|---|---|
| pkg/types/internal_meta.go | Introduces typed internal metadata and DeploymentRecordFromRaw decoding. |
| pkg/types/internal_meta_test.go | Verifies internal meta JSON shape and that RawObject does not expose it. |
| pkg/types/fingerprint.go | Updates fingerprint dependency resolution to use DeploymentRecord (needs nil-safety fixes). |
| pkg/types/fingerprint_test.go | Adjusts tests to build ApplyInput with DeploymentRecord. |
| pkg/types/adapter.go | Updates adapter contracts to accept DeploymentRecord and return typed internal meta + runtime status. |
| pkg/runtimes/kagent/translate_test.go | Updates tests for new Deployment status type and DeploymentRecord inputs. |
| pkg/runtimes/kagent/status.go | Switches from status.details metadata to internal meta + typed runtime status. |
| pkg/runtimes/kagent/discover.go | Emits discovery results with typed internal meta instead of runtimeMetadata map. |
| pkg/runtimes/kagent/discover_test.go | Updates discovery tests to assert internal meta fields. |
| pkg/runtimes/kagent/adapter.go | Removes legacy runtime-metadata constants; updates Apply/Remove to use internal meta/runtime status. |
| pkg/runtimes/kagent/adapter_test.go | Updates adapter tests to assert internal meta + runtime status instead of details metadata. |
| pkg/registry/v1alpha1store/tables.go | Enables internal meta persistence for Deployment/Runtime stores. |
| pkg/registry/v1alpha1store/store.go | Adds store option WithInternalMeta, patches internal_meta, and extends row scanning/selection. |
| pkg/registry/v1alpha1store/store_test.go | Adds coverage for PatchInternalMeta and PatchStatusAndMeta. |
| pkg/registry/v1alpha1store/migrations/016_internal_meta.up.sql | Adds internal_meta column to runtimes/deployments tables. |
| pkg/registry/v1alpha1store/migrations/016_internal_meta.down.sql | Drops internal_meta column on rollback. |
| pkg/registry/v1alpha1store/internal_meta_test.go | Tests option enablement and nil rejection for meta patch APIs. |
| pkg/registry/v1alpha1store/helpers.go | Extends scanRow to populate RawObject.InternalMeta. |
| pkg/api/v1alpha1/status.go | Removes Status.Details and adds typed patchers for Deployment/Runtime status. |
| pkg/api/v1alpha1/status_test.go | Removes tests that covered the deleted status.details behavior. |
| pkg/api/v1alpha1/runtime.go | Introduces RuntimeStatus as the public status type for Runtime. |
| pkg/api/v1alpha1/runtime_status_test.go | Adds storage round-trip test for RuntimeStatus. |
| pkg/api/v1alpha1/object.go | Adds RawObject.InternalMeta with json:"-"/yaml:"-". |
| pkg/api/v1alpha1/deployment.go | Introduces DeploymentStatus and DeploymentRuntimeStatus in the public API. |
| pkg/api/v1alpha1/accessors.go | Updates status marshal/unmarshal to support typed Runtime/Deployment status shapes. |
| internal/registry/service/deployment/adapter_resolver.go | Wraps log calls with DeploymentRecord for adapter interface compatibility. |
| internal/registry/runtimes/noop/adapter.go | Removes runtime metadata writing from noop adapter. |
| internal/registry/runtimes/noop/adapter_test.go | Updates noop adapter tests for DeploymentRecord inputs and removed runtime metadata. |
| internal/registry/runtimes/microsoft/discovery.go | Switches discovery results to use typed internal meta. |
| internal/registry/runtimes/microsoft/discovery_test.go | Updates expectations to internal meta (and removes metadata-normalization assertions). |
| internal/registry/controller/reconciler.go | Switches controller persistence from status.details to internal meta + typed DeploymentStatus patching. |
| internal/registry/controller/reconciler_integration_test.go | Updates integration tests to assert internal meta persistence instead of status.details. |
| internal/registry/controller/discovery.go | Moves discovery miss counter + runtime identity to internal meta; patches typed Deployment status/runtime. |
| internal/registry/controller/discovery_integration_test.go | Updates discovery integration tests to validate internal meta persistence and miss counter behavior. |
| internal/mcp/registryserver/server_test.go | Replaces old RawMessage/details schema test with DeploymentRuntimeStatus schema coverage. |
| internal/mcp/registryserver/server_integration_test.go | Updates tool schema validation fixture to use typed deployment runtime status. |
| internal/cli/common/deployments.go | Removes runtimeMetadata/details from CLI projection; updates error derivation for new DeploymentStatus. |
| internal/cli/common/deployments_test.go | Updates tests for DeploymentStatus wrapper and removes runtimeMetadata projection tests. |
Review details
Suppressed comments (1)
pkg/types/fingerprint.go:271
- This dereferences
in.Deployment.Deploymentwithout checking whetherin.Deploymentis nil. Since this function already supportsin.Deployment == nilearlier, this can panic when evaluating harness-related dependencies.
- Files reviewed: 38/38 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Persist typed Deployment and Runtime internal metadata outside the public status API. Add explicit status and metadata patch contracts, expose typed Deployment runtime status, and remove the opaque status details bag. ``` /kind feature ``` ```release-note Deployment and Runtime internal state is now stored separately from public status. ``` Signed-off-by: Shashank Ram <shashank.ram@solo.io>
Description
Persist typed Deployment and Runtime internal metadata outside the public status API. Add explicit status and metadata patch contracts, expose typed Deployment runtime status, and remove the opaque status details bag.
Change Type
/kind feature
Changelog
Additional Notes
NONE