Conversation
Closes apache#6567 Made-with: Cursor
squakez
left a comment
squakez
left a comment
There was a problem hiding this comment.
Thanks for the work. However, this is not really changing the dynamic creation of the actions. It really complicates that by generating an additional interface that is not really needed.
What I'd propose is to move the scope of the actions array from the func to the struct instead. In this way we just delegate their creation upon initialization instead of having them recreated at every reconcile loop. The only special case is where we have to filter based on a flag. In that case we can think to have 3 arrays and use the one specific to each flag type.
Keep the catalog controller actions initialized once on the reconciler, but stop mutating shared action instances with request-scoped logger state during each reconcile. The client is injected once at action creation time, while the reconcile-specific logger is now passed through context so the concurrent catalog controller can safely reuse its action list. Constraint: catalog-controller runs with MaxConcurrentReconciles enabled Rejected: recreating actions on every reconcile | goes against the requested initialization-time action setup Confidence: high Scope-risk: narrow Reversibility: clean Directive: Shared controller actions must not retain per-request mutable state unless they are concurrency-safe Tested: go test ./pkg/controller/catalog Tested: go test -race ./pkg/controller/catalog Tested: go test ./pkg/controller/integration ./pkg/controller/integrationkit ./pkg/controller/pipe Not-tested: integrationplatform package tests in this environment (missing mvn)
Update the remaining controllers that still stored action factories on the reconciler and rebuilt action slices during each reconcile. Integration, integrationkit, integrationplatform, and pipe now keep initialized action arrays directly on the struct, with integration selecting between the prebuilt synthetic and non-synthetic lists. Constraint: reviewer requested initialization-time action setup without an extra factory interface Rejected: keeping actionFactory on the reconciler | still recreates actions on each reconcile and preserves the unwanted indirection Confidence: high Scope-risk: narrow Reversibility: clean Directive: When controller actions are intended to be fixed at initialization time, store Action instances directly instead of factories Tested: go test ./pkg/controller/integration ./pkg/controller/integrationkit ./pkg/controller/pipe Tested: go test ./pkg/controller/integrationplatform -run TestActions Tested: go test ./pkg/controller/catalog Not-tested: full integrationplatform package in this environment (missing mvn)
Thanks for the feedback. I aligned the implementation with your suggestion by moving the actions from function scope to the reconciler struct, so they are initialized once instead of being recreated on every reconcile. For the Integration controller, I handled the flag-specific case by preparing dedicated action arrays for the synthetic and non-synthetic paths and selecting the appropriate one during reconcile. While doing that, I also fixed a concurrency issue in the catalog controller, where shared action instances could otherwise carry request-scoped mutable state across concurrent reconciliations. |
squakez
left a comment
squakez
left a comment
There was a problem hiding this comment.
Thanks. This version looks better but it still requires some changes.
|
✔️ Unit test coverage report - coverage increased from 62.2% to 62.3% (+0.1%) |
…e to init Move the integration controller's action arrays from function-scoped variables recreated on every reconcile to struct fields initialized once at reconciler creation. Use a shared baseActions slice to eliminate duplication between the synthetic and non-synthetic action lists. Drop the catalog controller changes from this PR as requested by the reviewer — any concurrency optimization there will be handled in a separate PR. Remove trivial action-type-only tests for integrationkit, integrationplatform, and pipe controllers that provided no real business logic coverage. Restore the original integration controller tests (TestIsIntegrationUpdated, TestReadinessTimestampCalculation, TestDeploymentTimestampIsSet) that were inadvertently removed.
squakez
left a comment
squakez
left a comment
There was a problem hiding this comment.
Nice work, thanks! I think now it's fine. If you can, please just rename a variable to better define the meaning of those actions. Also, about the Catalog, I think you could also apply only the actions refactoring to be completely consistent with this new pattern.
|
✔️ Unit test coverage report - coverage increased from 62.2% to 62.3% (+0.1%) |
squakez
left a comment
squakez
left a comment
There was a problem hiding this comment.
Great stuff, thanks for the contribution!
|
✔️ Unit test coverage report - coverage increased from 62.2% to 62.3% (+0.1%) |
2 participants
Summary
This PR addresses #6567 by moving controller action declarations from reconcile-time to reconciler initialization.
actionFactoryin controller reconcilers and initialize action factories innewReconciler(...)[]Action{...}declarations inReconcile(...)withnewActions(...)creationcatalogintegrationintegrationkitintegrationplatformpipeCloses #6567
Test Plan
go test ./pkg/controller/integration/ ./pkg/controller/catalog/ ./pkg/controller/integrationkit/ ./pkg/controller/integrationplatform/ ./pkg/controller/pipe/ -run TestNewActions -vgo vet ./pkg/controller/integration/ ./pkg/controller/catalog/ ./pkg/controller/integrationkit/ ./pkg/controller/integrationplatform/ ./pkg/controller/pipe/./bin/golangci-lint run --config .golangci.yml --timeout 5m ./pkg/controller/integration/ ./pkg/controller/catalog/ ./pkg/controller/integrationkit/ ./pkg/controller/integrationplatform/ ./pkg/controller/pipe/