Repository navigation
refactor(bridge): move actuation state tracking out of helm_manager.go - #1070
VLukiantsev merged 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the K8sHelmManager by extracting actuation state tracking and cold-start hydration logic into a new file actuation_state.go, along with moving corresponding tests and RBAC template rendering tests into separate files. It also simplifies the component uninstallation process. The review feedback highlights several critical issues where refactoring removed defensive nil checks, introducing potential runtime panics. Specifically, evaluating method values on a nil receiver in Delete1PComponent and Purge1PComponent will trigger immediate panics, and several map writes in the newly created actuation_state.go lack lazy initialization guards for activeState and actuatedGenerations.
ebcaf3a to
b147440
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @pachopepe on commit ebcaf3a.
b147440 to
e32018c
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
Clean rebase detected with zero semantic patch lines changed.
Preserving prior approval by @pachopepe on commit ebcaf3a.
e32018c to
1e5cab1
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (2 lines) are within the 10-line grace delta.
Preserving prior approval by @pachopepe on commit ebcaf3a.
Pure move, no behavior change. Active/actuated generation tracking and cold-start hydration (IsComponentActuated, RecordActuation, IsComponentActive, HydrateFromCluster, HydrateComponentState and helpers) move to actuation_state.go, with their tests in actuation_state_test.go. The Helm chart RBAC template test, which does not exercise K8sHelmManager, moves to components_rbac_test.go. The bridge README now maps each K8sHelmManager file to its responsibility. Review with `git diff --color-moved=dimmed-zebra`. TAG=agy CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399
- Add forgetComponentLocked so Delete1PComponent, Purge1PComponent and Orphan1PComponent share one state-clearing path. Delete keeps the component recorded as inactive with its chart override; Purge and Orphan forget it entirely. - Delete1PComponent and Purge1PComponent were then identical apart from the values mutation (flagged by the dupl linter), so they now share uninstallComponent, parameterised by a componentTeardown. - Drop lazy map initialisation and nil checks in the actuation and hydration code: NewK8sHelmManager always initialises these maps, and the hydration callers already guarantee a non-nil manager and namespace. - Add TestK8sHelmManager_TeardownForgetsState to pin the per-teardown state semantics. TAG=agy CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399
…t in actuation_state Replace the legacy xmanager_cloud/logging/logging alias with the canonical xmanager_cloud/internal/logging import path to satisfy the google3 legacy import ratchet check. TAG=agy CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399
1e5cab1 to
832b737
Compare
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (2 lines) are within the 10-line grace delta.
Preserving prior approval by @pachopepe on commit ebcaf3a.
There was a problem hiding this comment.
🤖 Incremental PR Approval
PR author is a code owner and changes since last approval (2 lines) are within the 10-line grace delta.
Preserving prior approval by @pachopepe on commit ebcaf3a.
Summary
Fourth slice of the
helm_manager.gosplit. Moves actuation state tracking and cold-start hydration intoactuation_state.go. It then gives Delete, Purge and Orphan one shared state-clearing path, and one shared uninstall flow for Delete and Purge.Motivation & Context
Tracking which components are active and at which generation they were actuated, and rebuilding that state from the cluster at startup, is its own job, separate from running Helm commands.
Part 4 of 6 of the
helm_manager.gosplit stack.Key Changes
actuation_state.go:IsComponentActuated,RecordActuation,IsComponentActive,HydrateFromCluster,HydrateComponentStateand their helpers, with tests inactuation_state_test.go.K8sHelmManager, so it moves tocomponents_rbac_test.go.K8sHelmManagerfile to its job.forgetComponentLockedgives the three teardowns one state-clearing path. Delete keeps the component recorded as inactive, with its chart override. Purge and Orphan forget it entirely.dupllinter flagged them. So both now calluninstallComponent, configured by acomponentTeardown. This was planned for PR 5 but had to come in here to pass the linter.TestK8sHelmManager_TeardownForgetsStateto pin down what each teardown keeps.Assumptions
Uninstalled 1P component Helm release, with anactionfield. Log-based alerts on the old messages would need updating; I found none in the repo.Verification & Testing
bazel test //pkg/components/bridge:bridge_test --test_output=errors --runs_per_test=3: passes.bazel build //pkg/components/... //cmd/xmc_bridge/...: passes.git diff --color-moved=dimmed-zebra.Related Issues / Bugs
N/A
TAG=agy
CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399