Enqueue Bundle on ConfigMap/Secret event #397

ash2k · 2019-01-05T11:00:49Z

If there is a Deployment in a Bundle that uses ConfigMap/Secret and this object is updated, Bundle is enqueued for processing.
Fixes #394.

Not sure how to test this. Will think more.

Burr · 2019-01-05T23:25:15Z

pkg/controller/bundlec/controller.go

+			if configMapRef == nil {
+				continue
+			}
+			indexKeys = append(indexKeys, bySecretNamespaceNameIndexKey(namespace, configMapRef.Name))


this should be byConfigMapNamespaceNameIndexKey, right?

same with the statement below.

Doh, I checked it like 3 times and still got it wrong =) Thanks! Copy-pasting is bad.

benbarclay · 2019-01-06T23:03:30Z

pkg/controller/bundlec/controller.go

+}
+
+func byConfigMapNamespaceNameIndexKey(configMapNamespace, configMapName string) string {
+	return configMapNamespace + "/" + configMapName


Any specific reason you use this style over fmt.Sprintf?

It is not important in this case of course, but I think concatenation is faster - don't need to parse the formatting string. Also this code does not depend on the fmt package that way (also not important).

ash2k · 2019-01-06T23:56:59Z

I want to have some tests here, so I'm not merging it straight away.

ychen-atlassian · 2019-01-08T05:37:40Z

pkg/controller/bundlec/controller.go

@@ -110,6 +148,37 @@ func (c *Controller) Run(ctx context.Context) {
 	<-ctx.Done()
 }

+// lookupBundleByDeploymentByIndex returns a function that can be used to perform lookups of Bundles that contain
+// Deployment objects that reference ConfigMap/Secret objects.
+func (c *Controller) lookupBundleByDeploymentByIndex(byIndex byIndexFunc, indexName string, indexKey indexKeyFunc) func(runtime.Object) ([]runtime.Object, error) {


This could probably be a nice util function in ctrl because we do something very similar in basically everywhere we have a LookupHandler

I have checked the places where LookupHandler is used, but all of them are not like this one. They are simpler. We have duplication there and we should DRY it, but the logic is not the same as here. Or I'm missing something?

nilebox

@ash2k don't we need a similar fix for Secret -> ServiceInstance -> Bundle propagation?

nilebox · 2019-01-08T05:52:54Z

pkg/controller/bundlec/controller.go

 	c.crdContext, c.crdContextCancel = context.WithCancel(context.Background())
 	crdInf.AddEventHandler(&crdEventHandler{
 		controller: c,
 		watchers:   make(map[string]watchState),
 	})
-
+	deploymentInf := resourceInfs[apps_v1.SchemeGroupVersion.WithKind("Deployment")]


nit: I know that k/k monorepo doesn't have constants for resource kinds, but we could define our own constants and use them?

Yes. A separate PR that updates all places with constants would be nice.

ash2k · 2019-01-08T09:04:05Z

@ash2k don't we need a similar fix for Secret -> ServiceInstance -> Bundle propagation?

@nilebox yes, good catch. We need the same fix there. #402

If there is a Deployment in a Bundle that uses ConfigMap/Secret and this object is updated, Bundle is enqueued for processing.

ash2k · 2019-01-28T10:06:02Z

I've added a test, ready for re-review. No code modifications were required. I could write a unit test but that would require a massive amount of setup code. I tried to re-use the testCase type that we have, but substituting workqueue with a mock one didn't work because it's copied to the lookup handler before I can replace it (without re-doing all the plumbing).

I'm going to do the same for Secret->ServiceInstance once this is merged.

brjacobsatlassian · 2019-01-30T02:12:44Z

cmd/smith/app/bundle_controller.go

@@ -226,7 +227,10 @@ func (c *BundleControllerConstructor) New(config *ctrl.Config, cctx *ctrl.Contex
 		Broadcaster: broadcaster,
 		Recorder:    recorder,
 	}
-	cntrlr.Prepare(crdInf, resourceInfs)
+	err = cntrlr.Prepare(crdInf, resourceInfs)


🤔 this should have been a warning before...

It wasn't returning an error before this PR.

brjacobsatlassian · 2019-01-30T02:14:03Z

it/deployment_dependencies_test.go

+			Resources: []smith_v1.Resource{
+				{
+					Name: deploymentResourceName,
+					Spec: smith_v1.ResourceSpec{


We're going to have to make builders for this stuff or put it in files. I'm realizing there's.... a lot of boilerplate.

ash2k added the enhancement label Jan 5, 2019

ash2k self-assigned this Jan 5, 2019

ash2k requested review from Burr and a team January 5, 2019 11:00

Burr reviewed Jan 5, 2019

View reviewed changes

Burr previously approved these changes Jan 6, 2019

View reviewed changes

benbarclay previously approved these changes Jan 6, 2019

View reviewed changes

ychen-atlassian reviewed Jan 8, 2019

View reviewed changes

ychen-atlassian previously approved these changes Jan 8, 2019

View reviewed changes

nilebox previously approved these changes Jan 8, 2019

View reviewed changes

ash2k mentioned this pull request Jan 8, 2019

Propagate events Secret -> ServiceInstance/ServiceBinding -> Bundle #402

Closed

ash2k added 2 commits January 26, 2019 17:59

Enqueue Bundle on ConfigMap/Secret event

d91df85

If there is a Deployment in a Bundle that uses ConfigMap/Secret and this object is updated, Bundle is enqueued for processing.

Use correct functions for configmap case

d089fa8

ash2k dismissed stale reviews from nilebox, ychen-atlassian, benbarclay, and Burr via 51f9a66 January 27, 2019 11:43

ash2k force-pushed the enqueue-bundle-on-secret-configmap-change branch from 974f4e6 to 51f9a66 Compare January 27, 2019 11:43

ash2k changed the title ~~Enqueue Bundle on ConfigMap/Secret event~~ WIP Enqueue Bundle on ConfigMap/Secret event Jan 27, 2019

ash2k force-pushed the enqueue-bundle-on-secret-configmap-change branch from 51f9a66 to 8c775cb Compare January 28, 2019 09:58

ash2k changed the title ~~WIP Enqueue Bundle on ConfigMap/Secret event~~ Enqueue Bundle on ConfigMap/Secret event Jan 28, 2019

Integration test for Secret/ConfigMap->Deployment->Bundle index

9d2ba54

ash2k force-pushed the enqueue-bundle-on-secret-configmap-change branch from 8c775cb to 9d2ba54 Compare January 29, 2019 03:39

brjacobsatlassian approved these changes Jan 30, 2019

View reviewed changes

ash2k merged commit f7b5065 into master Jan 30, 2019

ash2k deleted the enqueue-bundle-on-secret-configmap-change branch January 30, 2019 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enqueue Bundle on ConfigMap/Secret event #397

Enqueue Bundle on ConfigMap/Secret event #397

ash2k commented Jan 5, 2019

Burr Jan 5, 2019

ash2k Jan 6, 2019

benbarclay Jan 6, 2019

ash2k Jan 6, 2019

ash2k commented Jan 6, 2019

ychen-atlassian Jan 8, 2019

ash2k Jan 29, 2019

nilebox left a comment

nilebox Jan 8, 2019

ash2k Jan 8, 2019

ash2k Jan 8, 2019

ash2k commented Jan 8, 2019

ash2k commented Jan 28, 2019

brjacobsatlassian Jan 30, 2019

ash2k Jan 30, 2019

brjacobsatlassian Jan 30, 2019

Enqueue Bundle on ConfigMap/Secret event #397

Enqueue Bundle on ConfigMap/Secret event #397

Conversation

ash2k commented Jan 5, 2019

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

ash2k commented Jan 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox left a 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

ash2k commented Jan 8, 2019

ash2k commented Jan 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment