Controller #2

glyn · 2019-10-09T15:36:48Z

No description provided.

Allow for compilation using Go 1.12 and later.

scothis

I’ll need to take a deeper look at the unimap in the morning, but here’s my high level feedback

Dockerfile

README.md

api/v1alpha1/groupversion_info.go

api/v1alpha1/imagemap_types.go

config/default/kustomization.yaml

headache.json

pkg/unimap/composite.go

controllers/imagemap_controller.go

This was the result of running: mkdir hack cp <license header> hack/boilerplate.go.txt kubebuilder init --domain imagerelocation.pivotal.io where `kubebuilder version` returns: Version: version.Version{KubeBuilderVersion:"2.0.0", KubernetesVendor:"1.14.1", GitCommit:"b31cc5d96dbc91749eb49c2cf600bd951a46d4bd", BuildDate:"2019-08-22T23:39:53Z", GoOs:"unknown", GoArch:"unknown"}

This was the result of running: kubebuilder create api --group mapper --version v1alpha1 --kind ImageMap where `kubebuilder version` returns: Version: version.Version{KubeBuilderVersion:"2.0.0", KubernetesVendor:"1.14.1", GitCommit:"b31cc5d96dbc91749eb49c2cf600bd951a46d4bd", BuildDate:"2019-08-22T23:39:53Z", GoOs:"unknown", GoArch:"unknown"}

api/v1alpha1/imagemap_types.go

go.mod

controllers/imagemap_controller.go

glyn · 2019-11-14T04:46:57Z

@scothis I addressed your comments in a fresh commit, so PTAL. Please squash just this commit and the previous one and leave the rest unsquashed, or simply approve and leave it to me to merge.

scothis · 2019-11-14T15:53:58Z

api/v1alpha1/imagemap_types.go

+	Status corev1.ConditionStatus `json:"status" description:"status of the condition, one of True, False, Unknown"`
+
+	// ObservationTime records when the condition was observed
+	ObservedGeneration int64 `json:"observedGeneration,omitempty" description:"the ImageMap generation when the condition was observed"`


ObservedGeneration should be on the Status, not the condition. The generation is not a good indication of when something happened as the condition can and does change without a spec change. ObservedGeneration is only an indicator of whether or not the status is for the current spec. Also the doc is out of sync.

I'll move ObservedGeneration out of the condition and replace it with ObservationTime as I think it's useful to know the actual time that the condition was observed. Will sort out doc too.

scothis · 2019-11-14T16:05:07Z

controllers/imagemap_controller.go

+		r.Log.V(1).Info("mappings", "map", r.Map.Dump())
+	}
+
+	if statusUpdated {


it's far more robust to diff the new status with the existing status and then call update if there is a meaningful difference. This eliminates bugs and the complexity of manual tracking.

scothis · 2019-11-14T16:07:02Z

controllers/imagemap_controller.go

+	}
+
+	// copy the image map in case we need to mutate it
+	imageMap = *imageMap.DeepCopy()


typically you'd still hold a reference to the current value for later comparision

I think that's functionally equivalent to my current code, but I'll adopt the suggestion as it may make the code more maintainable.

controllers/imagemap_controller.go

scothis · 2019-11-14T16:13:03Z

controllers/imagemap_controller.go

+}
+
+func maybeUpdateCondition(conds *[]mapperv1alpha1.Condition, cond mapperv1alpha1.Condition) bool {
+	if len(*conds) != 1 || (*conds)[0] != cond {


Updating one condition should not remove every other conditions that may exist. It would be better to remove the existing condition from the array and then insert the new condition.

scothis · 2019-11-19T21:53:52Z

controllers/imagemap_controller.go

@@ -55,12 +56,16 @@ func (r *ImageMapReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	return resp, err
 }

-func (r *ImageMapReconciler) doReconcile(req ctrl.Request) (ctrl.Result, error) {
+func (r *ImageMapReconciler) reconcile(req ctrl.Request,
+	get func(context.Context, client.ObjectKey, runtime.Object) error,


now there are two ways to interact with the Client: the receiver and the arguments. It would be better to only have one way to interact with the client.

The mode of interaction with client.Client is as usual (receiver and arguments). What this code does is inject functional shims between reconcile and client.Client so that test shims can be provided in the unit test. I've used this pattern successfully elsewhere.

An approach I think you may be suggesting in another comment, which avoids the shims, is to mock an interface instead. This has the advantage that reconcile interacts with client receiver and arguments and so the unit test can be in a test package and can drive Reconcile instead. A downside is that there is more boilerplate in the unit test. See glyn#1 which shows the diff between this PR and that other approach.

I like the direction of glyn#1 much better. Left a few comments there for further improvements.

scothis · 2019-11-19T21:59:53Z

controllers/imagemap_controller_test.go

+	})
+
+	JustBeforeEach(func() {
+		result, err = reconciler.reconcile(req, get, update, status)


The canonical way to test controller-runtime reconcilers is to use envtest standup a test api-server.

That may be overkill for simple controllers, in that case, it be easier to create a fake/stub/mock client and create a reconciler with that client before calling the reconcile method?

I think an integration test may be warranted in due course: raised #8.

I think a unit test is desirable too, especially to test some error paths which would be tricky to drive in an integration test.

See the earlier comment about the mocking the client interface.

Based on the spike in vmware-archive/image-relocation#37. Deleted controllers/suite_test.go until we are ready to add an integration test.

scothis

let's not let perfect be the enemy of good. Let's get this merged and iterate

Initialise go module

445d632

Allow for compilation using Go 1.12 and later.

glyn self-assigned this Oct 9, 2019

glyn requested a review from scothis October 9, 2019 15:43

scothis reviewed Oct 10, 2019

View reviewed changes

glyn mentioned this pull request Oct 10, 2019

MutatingAdmissionWebhook for relocating image references vmware-archive/image-relocation#37

Closed

glyn added 2 commits October 10, 2019 11:32

glyn mentioned this pull request Oct 17, 2019

Implement a MutatingAdmissionWebhook to apply relocation mappings to pods #3

Open

glyn requested a review from scothis November 12, 2019 16:56

scothis suggested changes Nov 12, 2019

View reviewed changes

glyn requested a review from scothis November 14, 2019 04:45

scothis suggested changes Nov 14, 2019

View reviewed changes

glyn requested a review from scothis November 18, 2019 17:27

scothis reviewed Nov 19, 2019

View reviewed changes

Custom code and README

33eec9e

Based on the spike in vmware-archive/image-relocation#37. Deleted controllers/suite_test.go until we are ready to add an integration test.

glyn requested a review from scothis November 20, 2019 15:03

scothis approved these changes Nov 20, 2019

View reviewed changes

glyn merged commit 2754620 into vmware-archive:master Nov 20, 2019

glyn deleted the controller branch November 20, 2019 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller #2

Controller #2

glyn commented Oct 9, 2019

scothis left a comment

glyn commented Nov 14, 2019

scothis Nov 14, 2019

glyn Nov 18, 2019

scothis Nov 14, 2019

glyn Nov 18, 2019

scothis Nov 14, 2019

glyn Nov 18, 2019

scothis Nov 14, 2019

glyn Nov 18, 2019

scothis Nov 19, 2019

glyn Nov 20, 2019 •

edited

Loading

scothis Nov 20, 2019

scothis Nov 19, 2019

glyn Nov 20, 2019 •

edited

Loading

scothis left a comment

Controller #2

Controller #2

Conversation

glyn commented Oct 9, 2019

scothis left a comment

Choose a reason for hiding this comment

glyn commented Nov 14, 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

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

glyn Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyn Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

scothis left a comment

Choose a reason for hiding this comment

glyn Nov 20, 2019 •

edited

Loading

glyn Nov 20, 2019 •

edited

Loading