feat: pkg manager - #272
Conversation
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
…tants Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
Signed-off-by: Jakob Bergmeier <jakob.vetter@sap.com>
christophrj
left a comment
christophrj
left a comment
There was a problem hiding this comment.
Thanks @jakobve I really appreciate you taking the time to create this PR ❤️ I see a lot of really nice improvements, especially with the result handling 👍 Let's use the opportunity to revisit naming and structure while the PR is still in draft 🚀
| helmv2 "github.com/fluxcd/helm-controller/api/v2" | ||
| sourcev1 "github.com/fluxcd/source-controller/api/v1" | ||
| "github.com/openmcp-project/controller-utils/pkg/clusters" | ||
| "github.com/openmcp-project/opencontrolplane-runtime/testdata/api/v1alpha1" |
There was a problem hiding this comment.
The manager should not depend on the runtime testdata.
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
|
|
||
| "github.com/openmcp-project/controller-utils/pkg/manager" | ||
| "github.com/openmcp-project/opencontrolplane-runtime/pkg/serviceprovider/clusteraccess" |
There was a problem hiding this comment.
We should not depend on the runtime.
| // Interval defines the OCIRepository and HelmRelease reconcile intervals. | ||
| Interval time.Duration | ||
| // ClusterContext of the current reconciliation context. | ||
| ClusterContext clusteraccess.ClusterContext |
There was a problem hiding this comment.
Let's replace this with a kubeconfig field to get rid of the runtime dependency.
| require ( | ||
| github.com/evanphx/json-patch/v5 v5.9.11 | ||
| github.com/go-logr/logr v1.4.4 | ||
| github.com/fluxcd/helm-controller/api v1.6.3 |
There was a problem hiding this comment.
looking at these flux imports I wonder if a new extensibility-utils module would be a better fit than controller-utils. @maximiliantech what do you think?
| @@ -0,0 +1,76 @@ | |||
| package manager | |||
There was a problem hiding this comment.
Lets move secrets into a dedicated package similar to the flux functionality. This way we clearly distinguish between the base manager part vs the (convenience) functionality provided on top of the manager.
| // ResourceStatusWriter is implemented by a consumer's CRD-embedded resource type. | ||
| // ProjectResources drives it to populate a consumer-owned status entry from a | ||
| // framework ManagedResource, keeping pkg/manager decoupled from any CRD schema. | ||
| type ResourceStatusWriter interface { |
There was a problem hiding this comment.
Imo we don't need this but let's discuss (see comment below).
| // ResourceRef identifies a managed resource. It is passed to a consumer's | ||
| // ResourceStatusWriter so the consumer can decide how to store the fields | ||
| // (e.g. whether to represent an empty Namespace or APIGroup as nil). | ||
| type ResourceRef struct { |
There was a problem hiding this comment.
Let's discuss if this can be removed (see comments below)
| } | ||
|
|
||
| // OrphanCleaner removes any previously managed objects that are no longer part of the desired state. | ||
| type OrphanCleaner interface { |
There was a problem hiding this comment.
Lets rename this to Cleaner
| } | ||
|
|
||
| // NewOrphanCleaner removes redundant objects in the given target namespace. | ||
| func NewOrphanCleaner[T client.ObjectList](cluster ManagedCluster, serviceProvider string, namespace string, clType CleanerType[T]) OrphanCleaner { |
There was a problem hiding this comment.
Let's get rid of the Orphan prefix (also for the filename)
|
|
||
| // ManageFluxResourcesParams groups all parameters needed to register the | ||
| // Flux resources (OCIRepository + HelmRelease) on a cluster. | ||
| type ManageFluxResourcesParams struct { |
There was a problem hiding this comment.
Let's rename ManageFluxResourceParams to ResourceConfig and I would probably want to restructure this into ociRepo and helmRelease options for users to easier understand which parameter belongs to which resource.
We should also allow users to add additional configuration to customize the result beyond our required parameters, e.g.:
// HelmReleaseMutateFn allows advanced customization if required
HelmReleaseMutateFn func(s *helmv2.HelmReleaseSpec) error
// OCIRepoMutateFn allows advanced customization if required
OCIRepoMutateFn func(s *sourcev1.OCIRepositorySpec) error
2 participants
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes openmcp-project/backlog#620
Special notes for your reviewer:
Related PR: openmcp-project/service-provider-external-secrets#124
Release note: