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.

Sign up for GitHub

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

Jump to bottom

Refactor flags to improve unit tests #876

Merged
merged 1 commit into from
Dec 28, 2020
Merged

Refactor flags to improve unit tests #876

merged 1 commit into from
Dec 28, 2020

Conversation

MarcelMue
Copy link
Member

@MarcelMue MarcelMue commented Dec 18, 2020 •
edited
Loading

Changes

This PR refactors the usage of flags for the controller. It now utilizes a config struct which offers multiple benefits, such as easier testing of flag values and more independence between unit tests.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Pass a config struct to the reconciler instead of using flags directly.

@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 18, 2020
@tekton-robot tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 18, 2020
@tekton-robot
Copy link

Hi @MarcelMue. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@MarcelMue
Copy link
Member Author

@dibyom Can you check if the direction here is okay? I tried to orient myself a bit towards tektoncd/pipelines - but need to clean up + write new tests still.

@dibyom
Copy link
Member

dibyom commented Dec 21, 2020

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 21, 2020
dibyom
dibyom reviewed Dec 21, 2020
View reviewed changes
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Thanks @MarcelMue This looks much cleaner (and testable) than before.
I have a couple of minor comments and looks like some changes need to be made in test/ to fix the build. Otherwise LGTM!

pkg/reconciler/v1alpha1/eventlistener/controller.go Show resolved Hide resolved
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go Outdated
@@ -111,8 +74,8 @@ type Reconciler struct {
eventListenerLister listers.EventListenerLister
serviceLister corev1lister.ServiceLister

// systemNamespace is the namespace where the reconciler is deployed
systemNamespace string
// config is the configuration passed in through flags.
Copy link
Member

Choose a reason for hiding this comment

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

very minor: config is the configuration options that the Reconciler accepts.

(We could decide to pass these in via configMaps ore something else in the future)

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go Outdated
@@ -396,7 +359,7 @@ func (r *Reconciler) reconcileDeployment(ctx context.Context, logger *zap.Sugare
return nil
}

func getDeployment(el *v1alpha1.EventListener) *appsv1.Deployment {
func (r *Reconciler) getDeployment(el *v1alpha1.EventListener) *appsv1.Deployment {
Copy link
Member

Choose a reason for hiding this comment

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

If we make this getDeployment(el *v1alpha1.EventListener, config Config) the dependency on the Config is clearer and the function is a bit easier to test (we don't have to setup an entire reconciler struct)

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go Outdated
@@ -483,7 +446,7 @@ func getDeployment(el *v1alpha1.EventListener) *appsv1.Deployment {
}
}

func getContainer(el *v1alpha1.EventListener) corev1.Container {
func (r *Reconciler) getContainer(el *v1alpha1.EventListener) corev1.Container {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, I think its a bit clearer to pass in the Config struct as a Param

@dibyom
Copy link
Member

dibyom commented Dec 21, 2020

One other small thing wold be a more descriptive commit message (something like Pass a Config struct to reconciler instead of directly using flags or something.

MarcelMue
MarcelMue commented Dec 26, 2020
View reviewed changes
pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go
@@ -995,12 +1027,29 @@ func TestReconcile(t *testing.T) {
Services: []*corev1.Service{elService},
ConfigMaps: []*corev1.ConfigMap{loggingConfigMap},
},
}, {
name: "eventlistener with SetSecurityContext false",
Copy link
Member Author

Choose a reason for hiding this comment

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

I added this unit test. It is now possible to set a config for each test, ensuring that flags are honored properly.

@dibyom
Copy link
Member

dibyom commented Dec 28, 2020

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 28, 2020
@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 28, 2020
@tekton-robot tekton-robot merged commit 26f5c4a into tektoncd:master Dec 28, 2020
@MarcelMue MarcelMue deleted the refactor-flags branch December 29, 2020 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dibyom dibyom dibyom left review comments

@savitaashture savitaashture Awaiting requested review from savitaashture

@wlynch wlynch Awaiting requested review from wlynch

Assignees

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarcelMue @tekton-robot @dibyom