chore: moves config map creation logic from Feature pkg #909

bartoszmajsak · 2024-03-08T21:23:46Z

Refactors cluster resource creation funcs

Cleans up structure of the resource creation logic by:

adding CreateConfigMap func to be used in service-mesh refs logic
moving MetaOption and related object "decorators" to a single place (meta.go)
introducing WithOwnerReference decorator
opening up secret creation function for optional meta decorators

Splits service mesh-related config map creation

In addition, it reuses generic purpose CreateConfigMap func introduced as part of this PR.

Adds config map creation test

and moves the suite to the package.

The aim of this commit is to clean up structure of the resource creation logic by: - adding CreateConfigMap func to be used in service-mesh refs logic - moving MetaOption and related object "decorators" to a single place (meta.go) - introducing WithOwnerReference decorator - opening up secret creation function for optional meta decorators

In addition it reuses generic purpose CreateConfigMap func introduced as part of this PR.

We have missed a few tests because of that...

and moved the suite to the package.

bartoszmajsak · 2024-03-08T21:26:13Z

Might be good to merge #907 first :)

bartoszmajsak · 2024-03-08T21:27:36Z

Makefile

@@ -321,7 +321,7 @@ toolbox: ## Create a toolbox instance with the proper Golang and Operator SDK ve
 	toolbox create opendatahub-toolbox --image localhost/opendatahub-toolbox:latest

 # Run tests.
-TEST_SRC=./controllers/... ./tests/integration/features/... ./pkg/feature/...
+TEST_SRC=./controllers/... ./tests/integration/features/... ./pkg/...


Will be gone when #907 is merged.

bartoszmajsak · 2024-03-08T21:29:10Z

controllers/dscinitialization/servicemesh_setup.go

@@ -110,7 +110,7 @@ func configureServiceMeshFeatures() feature.FeaturesProvider {
 			PostConditions(
 				feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace),
 				func(f *feature.Feature) error {
-					return feature.WaitForPodsToBeReady(f.Spec.Auth.Namespace)(f)
+					return feature.WaitForPodsToBeReady(handler.DSCInitializationSpec.ServiceMesh.Auth.Namespace)(f)


No need to use feature struct here.

pkg/feature/servicemesh/resources.go

bartoszmajsak · 2024-03-08T22:51:50Z

/retest

aslakknutsen · 2024-03-08T23:02:38Z

pkg/cluster/meta.go

+	}
+}
+
+func WithLabels(labels ...string) MetaOptions {


WithAnnotations is probably a good candidate here as well.

Sure, but we can add it when we need it. Or do you think we should just provide it?

aslakknutsen · 2024-03-08T23:03:28Z

pkg/cluster/meta.go

+	}
+}
+
+func WithLabels(labels ...string) MetaOptions {


In some other package probably, but maybe a higher level version as well for unified labeling; WithUnifedLables(dsci, dsc, component)?

aslakknutsen · 2024-03-08T23:07:12Z

pkg/cluster/cluster_operations_int_test.go

+				"config-regs",
+				"default",
+				data,
+				cluster.WithLabels("apps.kubernetes.io/part-of", "opendatahub"),


I like the DSL bit of it but could we incorporate the same type of functions into the Feature.Manifest processing for instance, so that we have 1 API here not two depending on what you're doing?

feature.Create().. .Metadata(WithLabels(), WithOwnerRef()) .Manifests()..

I like the idea. Can we do it as subsequent PR?

pkg/feature/servicemesh/resources.go

israel-hdez

OK, this makes sense to me, as ConfigMap creation code looks misplaced in the Feature.

israel-hdez · 2024-03-10T16:19:31Z

controllers/dscinitialization/servicemesh_setup.go

@@ -88,7 +88,7 @@ func configureServiceMeshFeatures() feature.FeaturesProvider {

 		cfgMapErr := feature.CreateFeature("mesh-shared-configmap").
 			For(handler).
-			WithResources(servicemesh.ConfigMaps).
+			WithResources(servicemesh.MeshRefs, servicemesh.AuthRefs).


I hope this resolves Wen feedback in a previos PR about that this ConfigMaps wasn't really a good name :-)

aslakknutsen · 2024-03-11T09:25:23Z

controllers/dscinitialization/servicemesh_setup.go

@@ -88,7 +88,7 @@ func configureServiceMeshFeatures() feature.FeaturesProvider {

 		cfgMapErr := feature.CreateFeature("mesh-shared-configmap").
 			For(handler).
-			WithResources(servicemesh.ConfigMaps).
+			WithResources(servicemesh.MeshRefs, servicemesh.AuthRefs).


@bartoszmajsak should we perhaps consider splitting servicemesh and auth?

openshift-ci · 2024-03-12T15:05:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aslakknutsen, israel-hdez
Once this PR has been reviewed and has the lgtm label, please assign zdtsw for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…-services#909) #### Refactors cluster resource creation funcs Cleans up the structure of the resource creation logic by: - adding CreateConfigMap func to be used in service-mesh refs logic - moving MetaOption and related object "decorators" to a single place (`meta.go`) - introducing `WithOwnerReference` decorator - opening up secret creation function for optional meta decorators #### Splits service mesh-related config map creation In addition, it reuses the generic purpose `CreateConfigMap` func introduced as part of this PR. #### Adds config map creation test and moves the suite to the package.

…io#909) #### Refactors cluster resource creation funcs Cleans up the structure of the resource creation logic by: - adding CreateConfigMap func to be used in service-mesh refs logic - moving MetaOption and related object "decorators" to a single place (`meta.go`) - introducing `WithOwnerReference` decorator - opening up secret creation function for optional meta decorators #### Splits service mesh-related config map creation In addition, it reuses the generic purpose `CreateConfigMap` func introduced as part of this PR. #### Adds config map creation test and moves the suite to the package. (cherry picked from commit d7074eb)

bartoszmajsak added 4 commits March 8, 2024 21:54

chore: splits service mesh related config map creation

d9cfcc3

In addition it reuses generic purpose CreateConfigMap func introduced as part of this PR.

fix(make): adds entire pkg/... to run tests

acfaf46

We have missed a few tests because of that...

chore: adds config map creation test

6aa8f9a

and moved the suite to the package.

openshift-ci bot requested review from ruivieira and tarilabs March 8, 2024 21:23

bartoszmajsak requested review from israel-hdez and aslakknutsen and removed request for ruivieira and tarilabs March 8, 2024 21:23

bartoszmajsak changed the title ~~move config map from feature~~ chore: moves config map creation logic from Feature pkg Mar 8, 2024

bartoszmajsak commented Mar 8, 2024

View reviewed changes

pkg/feature/servicemesh/resources.go Outdated Show resolved Hide resolved

aslakknutsen reviewed Mar 8, 2024

View reviewed changes

chore: splits refs configmaps in two separate funcs

45f7ce3

israel-hdez approved these changes Mar 10, 2024

View reviewed changes

openshift-ci bot assigned israel-hdez Mar 10, 2024

openshift-ci bot added the lgtm label Mar 10, 2024

aslakknutsen reviewed Mar 11, 2024

View reviewed changes

Merge branch 'incubation' into move-config-map-from-feature

0da636b

openshift-ci bot removed the lgtm label Mar 11, 2024

aslakknutsen approved these changes Mar 12, 2024

View reviewed changes

openshift-ci bot assigned aslakknutsen Mar 12, 2024

openshift-ci bot added the lgtm label Mar 12, 2024

bartoszmajsak merged commit 623285e into opendatahub-io:incubation Mar 12, 2024
6 of 7 checks passed

bartoszmajsak deleted the move-config-map-from-feature branch March 12, 2024 15:06

bartoszmajsak mentioned this pull request Mar 25, 2024

dead code #936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: moves config map creation logic from Feature pkg #909

chore: moves config map creation logic from Feature pkg #909

bartoszmajsak commented Mar 8, 2024 •

edited

Loading

bartoszmajsak commented Mar 8, 2024

bartoszmajsak Mar 8, 2024

bartoszmajsak Mar 8, 2024

bartoszmajsak commented Mar 8, 2024

aslakknutsen Mar 8, 2024

bartoszmajsak Mar 8, 2024

aslakknutsen Mar 8, 2024

aslakknutsen Mar 8, 2024

bartoszmajsak Mar 8, 2024

aslakknutsen Mar 9, 2024

israel-hdez left a comment

israel-hdez Mar 10, 2024

aslakknutsen Mar 11, 2024

openshift-ci bot commented Mar 12, 2024

chore: moves config map creation logic from Feature pkg #909

chore: moves config map creation logic from Feature pkg #909

Conversation

bartoszmajsak commented Mar 8, 2024 • edited Loading

Refactors cluster resource creation funcs

Splits service mesh-related config map creation

Adds config map creation test

bartoszmajsak commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartoszmajsak commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

israel-hdez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 12, 2024

bartoszmajsak commented Mar 8, 2024 •

edited

Loading