Repository navigation
refactor(bridge): move conditions and spec parsing out of reconciler.go - #1009
Merged
Merged
Conversation
reconciler.go had grown to ~1,500 lines and mixed several responsibilities (see the #992 review). This is the first layer of a pure-move split with no behavior or public API changes: - conditions.go: condition types/statuses, health reasons, Condition, findCondition (from dependency_readiness.go), setCondition, upsertCondition, healthConditionStatuses, and the CMS heartbeat throttling (shouldSyncToCMS, resetReadyTransitionTime). - spec_parser.go: unstructured spec/status decoding (ParseSpecFromUnstructured, ParseConditionsFromUnstructured, ParseWrapperRef, parameter/manifest/enum parsers, extract* accessors). - Matching unit tests moved 1:1 into conditions_test.go and spec_parser_test.go; top-level test count is unchanged (106). Review with: git diff --color-moved=dimmed-zebra TAG=agy CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399
There was a problem hiding this comment.
Code Review
This pull request refactors the bridge package by extracting condition-related logic and spec parsing helpers from reconciler.go and dependency_readiness.go into dedicated files: conditions.go and spec_parser.go, along with their respective unit tests. Feedback is provided regarding the parsing of Kubernetes condition timestamps in spec_parser.go, where using time.RFC3339 instead of time.RFC3339Nano can lead to silent parsing failures when fractional seconds are present, potentially causing write amplification to CMS and Cloud Spanner.
This was referenced Sep 29, 2026
VLukiantsev
marked this pull request as ready for review
September 29, 2026 07:39
janzak-dev
approved these changes
Oct 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part 1/4 of a pure-move stack splitting
pkg/components/bridge/reconciler.go(1492 lines) by responsibility. This PR moves Kubernetes status-condition helpers intoconditions.goand CR spec parsing intospec_parser.go, along with their tests. There are no behaviour, API or naming changes.Motivation & Context
reconciler.gomixes condition bookkeeping, spec parsing, the CMS transport, probing, 3P wrapper reconciliation and deletion/deactivation lifecycle. The #992 review agreed on a file layout and left a TODO at the top of the file. This stack applies that layout, pluscms_client.goandbridge_prober.goso that each file holds one type or concern. Each PR is a mechanical move so it can be reviewed with--color-moved.Review with:
git diff --color-moved=dimmed-zebra origin/main...origin/refactor/bridge-split-conditions-parserKey Changes
conditions.go:ConditionType*/ConditionStatus*/healthReason*constants,Condition,findCondition(moved fromdependency_readiness.go),setCondition,upsertCondition,healthConditionStatuses,defaultCMSHeartbeatInterval,shouldSyncToCMS,resetReadyTransitionTime.spec_parser.go:ParseSpecFromUnstructured,ParseConditionsFromUnstructured,ParseWrapperRef, and theparse*/extract*helpers.conditions_test.goandspec_parser_test.go.BUILD.bazelsrcs updated.Assumptions
findConditionmoved fromdependency_readiness.gotoconditions.gobecause it's a condition helper. It's the only symbol moved from outsidereconciler.go— impact if wrong: a one-line move back.Verification & Testing
bazel test //pkg/components/bridge:bridge_test,bazel build //pkg/components/... //cmd/xmc_bridge/...,./.githooks/pre-commitRelated Issues / Bugs
Follow-up to the #992 review.
TAG=agy
CONV=25ae2278-aa0f-44db-af8a-4f9deda0d399