Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate mutator controller logic #1474

Merged
merged 1 commit into from
Aug 9, 2021

Conversation

maxsmythe
Copy link
Contributor

Fixes #1449

Signed-off-by: Max Smythe smythe@google.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #

Special notes for your reviewer:

@maxsmythe maxsmythe changed the title Deduplicate mutator controller logic. Deduplicate mutator controller logic Aug 4, 2021
@maxsmythe maxsmythe force-pushed the dedupe-mutators branch 2 times, most recently from 7a22df5 to b028142 Compare August 4, 2021 02:44
kind := "Assign"
newObj := func() client.Object { return &mutationsv1alpha1.Assign{} }
newMutator := func(obj client.Object) (types.Mutator, error) {
assign := obj.(*mutationsv1alpha1.Assign)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lint error here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabled linting. I think we want to panic, which will expose type mismatches during testing.

}

// Reconciler reconciles mutator objects.
type Reconciler struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nice for Reconciler type declaration to be above newReconciler() in the code, for readability

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on what you're looking for. If you want to know "how do I add a reconciler", it's better to have the newReconciler at the top, as consumers of this generally don't care how the reconciler is implemented.

The current order of things sticks as closely to the kubebuilder generated output as we can, which hopefully makes the code more readable to those with that context.

tracker *readiness.Tracker,
getPod func() (*corev1.Pod, error),
kind string,
newObj func() client.Object,
Copy link
Contributor

@julianKatz julianKatz Aug 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more descriptive name could be nice here. Something like mutationType. newMutator could be MutatorFor or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I workshopped some names, let me know what you think

@maxsmythe
Copy link
Contributor Author

@ritazh @julianKatz addressed comments and pushed changes.

GetPod: a.GetPod,
MutationSystem: a.MutationSystem,
Kind: "Assign",
NewMutationObj: func() client.Object { return &mutationsv1alpha1.Assign{} },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better 👍🏼

@maxsmythe
Copy link
Contributor Author

LGTY @ritazh ?

Fixes open-policy-agent#1449

Signed-off-by: Max Smythe <smythe@google.com>
@maxsmythe maxsmythe merged commit 407611a into open-policy-agent:master Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor duplicate code: assign_controller.go and assignmetadata_controller.go
5 participants