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

Pick up genreconciler #635

Closed
mattmoor opened this issue Jun 25, 2020 · 3 comments · Fixed by #733
Closed

Pick up genreconciler #635

mattmoor opened this issue Jun 25, 2020 · 3 comments · Fixed by #733
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@mattmoor
Copy link
Member

I did this for pipelines, and can either help or guide this here, but the general idea is that you can decorate types with:

// +genreconciler

... and we will generate a type reconciler shell for it, which hold most of Knative's upstream controller best practices.

You can see an example of this migration here.

It's also notable that if you jump all the way forward (tektoncd/pipeline#2846 👀 ) you will also start having the ability to enable fine-grained leader election and sharded reconciliation with little-to-no code changes 🤩

cc @vdemeester @afrittoli @imjasonh

@dibyom
Copy link
Member

dibyom commented Jun 26, 2020

I think this might also address #627

I'll try to pick this up next week...unless someone beats me to it 😉

@vdemeester
Copy link
Member

/kind feature

@tekton-robot tekton-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 26, 2020
@dibyom dibyom self-assigned this Jul 1, 2020
dibyom added a commit to dibyom/triggers that referenced this issue Jul 6, 2020
Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 6, 2020
Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 7, 2020
The update to knative is to pick up genreconciler and in turn requires us to update our pipelines dependency to v0.13.2. I had to update the hack/ scripts and remove some old pinned dependencies, and drop some deprecated functions and flags.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 7, 2020
The update to knative is to pick up genreconciler and in turn requires us to update our pipelines dependency to v0.13.2. I had to update the hack/ scripts and remove some old pinned dependencies, and drop some deprecated functions and flags.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 7, 2020
The update to knative is to pick up genreconciler and in turn requires us to update our pipelines dependency to v0.13.2. I had to update the hack/ scripts and remove some old pinned dependencies, and drop some deprecated functions and flags.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 7, 2020
The update to knative is to pick up genreconciler and in turn requires us to
update our pipelines dependency to v0.13.2. I had to update the hack/ scripts,
remove some old pinned dependencies, and drop some deprecated functions and
flags. To get the webhook cert generation to work again, I had to replace
`sharedmain.MainWithContext` with `sharedmain.WebhookMainWihtConfig`.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 7, 2020
The update to knative is to pick up genreconciler and in turn requires us to
update our pipelines dependency to v0.13.2. I had to update the hack/ scripts,
remove some old pinned dependencies, and drop some deprecated functions and
flags. To get the webhook cert generation to work again, I had to replace
`sharedmain.MainWithContext` with `sharedmain.WebhookMainWihtConfig`.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Jul 8, 2020
The update to knative is to pick up genreconciler and in turn requires us to
update our pipelines dependency to v0.13.2. I had to update the hack/ scripts,
remove some old pinned dependencies, and drop some deprecated functions and
flags. To get the webhook cert generation to work again, I had to replace
`sharedmain.MainWithContext` with `sharedmain.WebhookMainWihtConfig`.

Needed for tektoncd#635 and fixes tektoncd#627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Jul 8, 2020
The update to knative is to pick up genreconciler and in turn requires us to
update our pipelines dependency to v0.13.2. I had to update the hack/ scripts,
remove some old pinned dependencies, and drop some deprecated functions and
flags. To get the webhook cert generation to work again, I had to replace
`sharedmain.MainWithContext` with `sharedmain.WebhookMainWihtConfig`.

Needed for #635 and fixes #627

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom dibyom added this to the Triggers v0.7.0 milestone Jul 8, 2020
@dibyom
Copy link
Member

dibyom commented Jul 10, 2020

I made some progress here: https://github.com/tektoncd/triggers/compare/master...dibyom:genreconciler?expand=1

I still need to fix up some of our reconciler tests that were testing unexported functions

dibyom added a commit to dibyom/triggers that referenced this issue Aug 14, 2020
We had separate tests for reconcileService and reconcileDeployment. These tests
were hard to port with the genreconciler update(tektoncd#635) which changes the Reconciler
interface. Instead of testing these unexported functions, I merged the test
cases for these into the existing TestReconcile function

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Aug 17, 2020
We had separate tests for reconcileService and reconcileDeployment. These tests
were hard to port with the genreconciler update(tektoncd#635) which changes the Reconciler
interface. Instead of testing these unexported functions, I merged the test
cases for these into the existing TestReconcile function

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Aug 19, 2020
We had separate tests for reconcileService and reconcileDeployment. These tests
were hard to port with the genreconciler update(#635) which changes the Reconciler
interface. Instead of testing these unexported functions, I merged the test
cases for these into the existing TestReconcile function

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Aug 28, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One  change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes tektoncd#635
dibyom added a commit to dibyom/triggers that referenced this issue Aug 28, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One  change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Aug 28, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One  change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Aug 28, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One  change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Sep 1, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler. One  change I had to make to use genreconciler
is to add a Finalizer for deleting the logging config maps when an
EventListener is deleted.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Sep 1, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler.

* Migrates the logic for deleting logging config maps to a finalizer
* Uses listers to get existing deployment, service, configMap instead
of making direct API calls.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Sep 2, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler.

* Migrates the logic for deleting logging config maps to a finalizer
* Uses listers to get existing deployment, service, configMap instead
of making direct API calls.

Fixes tektoncd#635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Sep 8, 2020
Using genreconciler from knative/pkg should reduce the amount of boilerplate we
need to write a reconciler.

* Migrates the logic for deleting logging config maps to a finalizer
* Uses listers to get existing deployment, service, configMap instead
of making direct API calls.

Fixes #635

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants