refactor: Drop internal/declarative/v2 package by LeelaChacha · Pull Request #3358 · kyma-project/lifecycle-manager

LeelaChacha · 2026-06-16T00:28:00Z

Relocates the contents of internal/declarative/v2 into the layer each piece belongs to, then deletes the package.

Reconciler + setup → internal/controller/manifest/
Spec → internal/manifest/spec/
Cached manifest parser → internal/manifest/parser/
Resource transforms → internal/service/manifest/render/
ExistsStateCheck → internal/manifest/statecheck/
ResourceList → private to internal/controller/manifest/

Drops the Object abstraction (transforms now take *v1beta2.Manifest directly), removes the generated mock_v2 package, consolidates duplicated constants onto shared.OwnedByFormat / shared.DefaultRemoteNamespace, lifts labelsremoval.NewManagedByLabelRemovalService to the composition root, turns SpecResolver into a consumer interface on the reconciler, inlines all collaborator interfaces alongside their consumer in controller.go, and pins the manifestctrl import alias in .golangci.yaml.

Closes #2745

Relocates everything that lived in the v2 package into the layer it belongs to per ADR 004/005: - Reconciler + setup → internal/controller/manifest/ - Spec struct → internal/manifest/spec/ - Cached manifest parser → internal/manifest/parser/ - Resource transforms + render-side helpers → internal/service/manifest/render/ - ExistsStateCheck → internal/manifest/statecheck/ - ResourceList → internal/controller/manifest/ (private) Drops the Object abstraction (replaced by *v1beta2.Manifest), removes the generated mock_v2 package, consolidates duplicated constants onto shared.OwnedByFormat / shared.DefaultRemoteNamespace, lifts labelsremoval.NewManagedByLabelRemovalService construction up to cmd/main.go (ADR 002), turns SpecResolver into a consumer interface on the reconciler (ADR 001), and pins the manifestctrl import alias. Closes kyma-project#2745

Re-add the manifestctrl alias to the two integration suite_test.go files where the formatter dropped it, satisfying the importas linter rule (.golangci.yaml line 64). Update unit-test-coverage-lifecycle-manager.yaml: - internal/service/manifest/render: 96 -> 84.1 (testObj shim removal + Object guard deletion lowered the line count denominator) - internal/manifest/statecheck: 71 -> 66.1 (ExistsStateCheck added) - internal/controller/manifest: 4.5 (new package floor)

- Centralise resource ID format: ResourceList.objectID delegates to shared.Resource.ID() so the controller and api/shared cannot drift. - Add unit tests for ExistsStateCheck (success + IgnoreNotFound + error). - Add unit tests for CachedManifestParser (cache hit/miss, eviction, deep-copy isolation, error path). - Add behavioural tests for DockerImageLocalizationTransform. - Lowercase the two error sentinels with no external callers (errManagerInErrorState, errResourceSyncDiffInSameOCILayer). - Drop the pointer indirection in CachedManifestParser.Parse. - Update coverage YAML floors: - internal/service/manifest/render: 84.1 -> 92.5 - internal/manifest/statecheck: 66.1 -> 74.2 (above the original 71) - internal/manifest/parser: 17.6 (new) - internal/manifest/spec: 92.3 (new entry; tests already existed)

It only exercises exported API; drop the //nolint:testpackage marker.

Copilot

Pull request overview

This PR removes the internal/declarative/v2 package by relocating its responsibilities into more appropriate layers (controller/manifest, manifest/spec+parser+statecheck, and service/manifest/render), updating call sites, tests, and documentation accordingly.

Changes:

Migrates reconciler/setup and related helper types into internal/controller/manifest, removing the v2 “declarative” abstraction layer.
Introduces dedicated internal/manifest/spec + internal/manifest/parser + internal/manifest/statecheck packages to replace v2 equivalents.
Refactors manifest rendering transforms into internal/service/manifest/render to operate directly on *v1beta2.Manifest.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
unit-test-coverage-lifecycle-manager.yaml	Adds coverage thresholds for newly introduced/relocated packages.
tests/integration/controller/manifest/suite_test.go	Updates integration suite wiring to new manifest controller + parser/render/statecheck packages.
tests/integration/controller/manifest/oci_reg_secret/suite_test.go	Same as above for OCI registry secret suite; adjusts imports and reconciler construction.
tests/integration/controller/manifest/custom_resource_check/suite_test.go	Same as above for custom resource check suite; injects managed label removal service.
tests/e2e/utils_test.go	Switches env var constant reference from declarative v2 to render package.
internal/service/manifest/render/transform.go	Adds render-local transform and parser interfaces (new abstraction boundary).
internal/service/manifest/render/service.go	Updates render service to consume `spec.Spec` + new transform/parser interfaces.
internal/service/manifest/render/service_test.go	Updates render service unit tests to use `spec.Spec` and `*v1beta2.Manifest` in transforms.
internal/service/manifest/render/image_pull_secret_transform.go	Moves/renames transform into render package and drops old Object type assertion.
internal/service/manifest/render/image_pull_secret_transform_test.go	Updates tests to new render package APIs/constants.
internal/service/manifest/render/docker_image_localization_test.go	Adds focused unit tests for image localization transform behavior.
internal/service/manifest/render/default_transforms.go	Moves default transforms into render package and consolidates constants onto `shared`.
internal/service/manifest/render/default_transforms_test.go	Updates tests to new transform signatures and package paths.
internal/manifest/statecheck/exists_state_check.go	Moves ExistsStateCheck into dedicated `statecheck` package.
internal/manifest/statecheck/exists_state_check_test.go	Adds unit tests for ExistsStateCheck behavior.
internal/manifest/spec/spec.go	Introduces new `spec.Spec` type replacing declarative v2 Spec.
internal/manifest/spec/resolver.go	Updates resolver to return `*spec.Spec` instead of v2 Spec.
internal/manifest/spec/resolver_test.go	Updates resolver tests for new `spec.Spec`.
internal/manifest/parser/cached_manifest_parser.go	Adds cached manifest parser implementation (TTL-based) in new package.
internal/manifest/parser/cached_manifest_parser_test.go	Adds unit tests for caching/deep-copy/eviction/error behavior.
internal/declarative/v2/spec.go	Removes declarative v2 Spec/SpecResolver types (deleted).
internal/declarative/v2/resource.go	Removes declarative v2 ResourceList implementation (deleted).
internal/declarative/v2/object.go	Removes declarative v2 Object abstraction (deleted).
internal/declarative/v2/mock/object.go	Removes generated mock package (deleted).
internal/declarative/v2/inmemory_rendered.go	Removes v2 in-memory parser implementation (deleted).
internal/controller/manifest/setup.go	Updates setup to use new manifest controller interfaces and injected label removal service.
internal/controller/manifest/resource_list.go	Reintroduces ResourceList under controller/manifest with ID consistency via `shared.Resource`.
internal/controller/manifest/resource_list_test.go	Updates tests to new package path and enables `t.Parallel()`.
internal/controller/manifest/controller.go	Refactors reconciler, inlines collaborator interfaces, and adopts new spec/parser/render/statecheck boundaries.
internal/controller/manifest/controller_test.go	Updates pruneResource tests to use `shared.DefaultRemoteNamespace`.
docs/contributor/resources/02-manifest.md	Updates documentation reference from declarative library to render service.
docs/contributor/02-controllers.md	Updates controller documentation to describe the new reconciliation flow.
cmd/main.go	Updates manifest reconciler wiring: new parser, render composition, and injected managed-by label removal service.
cmd/composition/service/manifest/render/render.go	Updates render composition to use new parser package and render transforms.
.golangci.yaml	Pins `manifestctrl` alias for controller/manifest and removes declarative v2 alias entries.

Comments suppressed due to low confidence (3)

internal/controller/manifest/controller.go:253

prepareReconcile no longer special-cases accessmanager.ErrAccessSecretNotFound. This changes behavior vs the delete path (and the previous install path) and can leave Manifests stuck in error/requeue without clearing finalizers/metrics when the SKR access secret is gone. Consider routing this error through cleanupManifest, same as prepareDeleteReconcile.
internal/service/manifest/render/image_pull_secret_transform.go:17
Typo in error message: "already exits" -> "already exists".
internal/manifest/statecheck/exists_state_check.go:14
The doc comment says StateReady is reported once resources are "retrievable", but the implementation explicitly ignores NotFound errors (treating missing resources as ready). Updating the comment to match the behavior will avoid confusion for future callers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…er package

…ller Helpers in the install/delete pipeline previously returned (T, ctrl.Result, bool, error). Replace with (T, *stepResult) where *stepResult wraps the (Result, Err) the caller must return on a short-circuit. A nil pointer means continue to the next step. This avoids overloading error for non-failure short-circuits (ssaSpec, updateManifest, ErrDeletionNotFinished, unmanaged cleanup all requeue with Err == nil), and keeps the call sites two-valued without the errors.Is/As / zero-Result footguns of a structuredError approach.

c-pius

Thanks, this is a great step forward! Consider my comments optional and decide yourself if you want to apply something. I think we will anyway need to continue polishing this.

I am a bit uneasy about this change though. It was super hard to review this because so many things moved. Also, this is one of the most important parts in the code of KLM. We should quickly align with the team how we plan to release and test this in the live landscapes. Also we may need to be cautious when delivering this together with other high-priority features.

c-pius · 2026-06-23T10:55:24Z

+// SyncedOCIRefAnnotation is the annotation key on a Manifest CR that records
+// the OCI ref last successfully synced to the SKR. The reconciler uses it to
+// detect re-renders triggered by an OCI ref change.
+const SyncedOCIRefAnnotation = "sync-oci-ref"


I was this years old when I learned... :)

Rhetorical question, why is this an annotation and not status? 🥹

c-pius · 2026-06-23T11:22:27Z

+// SKRClientCache holds Kubernetes clients for SKR clusters keyed by the
+// manifest's cache key.
 type SKRClientCache interface {
 	GetClient(key string) *skrclient.SKRClient
 	AddClient(key string, client *skrclient.SKRClient)
 	DeleteClient(key string)
 }


As the next follow up we can try to use the same SKR client cache for Kyma and Manifest reconciler :)

Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>

c-pius · 2026-06-24T08:06:58Z

Holding until the regular KLM release was created Friday. We will then merge it and create an additional patch release to isolate this change.

Drop the misleading "shared by install/delete" wording — these helpers aren't shared between the install and delete paths; each runs only in its own pipeline. Reframe as the preparatory steps that run before spec resolution / resource rendering.

CachedManifestParser is consumed in exactly one place — Service in service.go. Move the interface declaration next to its sole consumer rather than keeping it in transform.go (which is named for, and now contains only, ResourceTransform). ResourceTransform stays in transform.go because multiple files in the package consume it.

LeelaChacha requested review from a team as code owners June 16, 2026 00:28

LeelaChacha temporarily deployed to restricted June 16, 2026 00:28 — with GitHub Actions Inactive

LeelaChacha changed the title ~~refactor: drop internal/declarative/v2 package~~ refactor: Drop internal/declarative/v2 package Jun 16, 2026

LeelaChacha commented Jun 16, 2026

View reviewed changes

Comment thread internal/manifest/statecheck/exists_state_check.go

LeelaChacha requested a deployment to restricted June 16, 2026 02:03 — with GitHub Actions Waiting

fix: use manifestctrl alias for NewReconciler call sites

d83d3ac

LeelaChacha requested a deployment to restricted June 16, 2026 02:32 — with GitHub Actions Waiting

LeelaChacha requested a deployment to restricted June 16, 2026 11:01 — with GitHub Actions Waiting

test: move resource_list_test.go to external package

14dfd43

It only exercises exported API; drop the //nolint:testpackage marker.

LeelaChacha temporarily deployed to restricted June 16, 2026 11:56 — with GitHub Actions Inactive

Merge branch 'main' into refactor/drop-declarative-v2-package

e884fe7

LeelaChacha enabled auto-merge (squash) June 16, 2026 13:22

LeelaChacha temporarily deployed to restricted June 16, 2026 13:22 — with GitHub Actions Inactive

LeelaChacha requested a review from Copilot June 16, 2026 13:41

Copilot started reviewing on behalf of LeelaChacha June 16, 2026 13:42 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

LeelaChacha disabled auto-merge June 18, 2026 10:16

chore: Merge upstream main and port deployer secret transform to rend…

9451e59

…er package

LeelaChacha temporarily deployed to restricted June 19, 2026 09:20 — with GitHub Actions Inactive

LeelaChacha enabled auto-merge (squash) June 19, 2026 09:22

chore: update coverage threshold for internal/controller/manifest

130dff3

LeelaChacha temporarily deployed to restricted June 19, 2026 12:24 — with GitHub Actions Inactive

Tomasz-Smelcerz-SAP reviewed Jun 19, 2026

View reviewed changes

Comment thread internal/controller/manifest/controller.go Outdated

LeelaChacha requested a deployment to restricted June 22, 2026 14:15 — with GitHub Actions Waiting

Merge branch 'main' into refactor/drop-declarative-v2-package

59662d3

LeelaChacha temporarily deployed to restricted June 22, 2026 14:17 — with GitHub Actions Inactive

Merge branch 'main' into refactor/drop-declarative-v2-package

7a9ee05

c-pius temporarily deployed to restricted June 23, 2026 10:45 — with GitHub Actions Inactive

c-pius reviewed Jun 23, 2026

View reviewed changes

chore: Apply suggestions from code review

b89080e

Co-authored-by: Christoph Schwägerl <acc.pius@mailbox.org>

LeelaChacha requested a deployment to restricted June 23, 2026 18:34 — with GitHub Actions Waiting

c-pius added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2026

LeelaChacha requested a deployment to restricted June 24, 2026 10:22 — with GitHub Actions Waiting

LeelaChacha force-pushed the refactor/drop-declarative-v2-package branch from 8a4f6ff to 8dd2908 Compare June 24, 2026 11:10

LeelaChacha requested a deployment to restricted June 24, 2026 11:10 — with GitHub Actions Waiting

LeelaChacha temporarily deployed to restricted June 24, 2026 11:16 — with GitHub Actions Inactive

Merge branch 'main' into refactor/drop-declarative-v2-package

0fab203

LeelaChacha temporarily deployed to restricted June 24, 2026 11:54 — with GitHub Actions Inactive

c-pius approved these changes Jun 24, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Drop internal/declarative/v2 package#3358

refactor: Drop internal/declarative/v2 package#3358
LeelaChacha wants to merge 15 commits into
kyma-project:mainfrom
LeelaChacha:refactor/drop-declarative-v2-package

LeelaChacha commented Jun 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

c-pius left a comment

Uh oh!

c-pius Jun 23, 2026

Uh oh!

Uh oh!

c-pius Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

c-pius commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

LeelaChacha commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

c-pius left a comment

Choose a reason for hiding this comment

Uh oh!

c-pius Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

c-pius Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

c-pius commented Jun 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LeelaChacha commented Jun 16, 2026 •

edited

Loading