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

✨ Simplify usage of EnqueueRequestsFromMapFunc #1119

Merged
merged 1 commit into from Aug 6, 2020
Merged

✨ Simplify usage of EnqueueRequestsFromMapFunc #1119

merged 1 commit into from Aug 6, 2020

Conversation

alvaroaleman
Copy link
Member

@alvaroaleman alvaroaleman commented Aug 6, 2020 •
edited

Makes creation of custom mappers a lot simpler

/assign @vincepri

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 6, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2020
alvaroaleman
alvaroaleman commented Aug 6, 2020
View reviewed changes
pkg/handler/enqueue_mapped.go Outdated
ToRequests: ToRequestsFunc(mapFN),
}
}

var _ EventHandler = &EnqueueRequestsFromMapFunc{}
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we can completely unexport EnqueueRequestsFromMapFunc. It doesn't provide any advantage over using FromMapFunc and is a lot more annoying to construct

Copy link
Member

Choose a reason for hiding this comment

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

+1 to that, better have a single way to use it than two and confuse users

Copy link
Member

Choose a reason for hiding this comment

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

Let's make that a ⚠️

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, PTAL

@vincepri
Copy link
Member

vincepri commented Aug 6, 2020

/milestone v0.7.x

@k8s-ci-robot k8s-ci-robot added this to the v0.7.x milestone Aug 6, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2020
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2020
vincepri
vincepri reviewed Aug 6, 2020
View reviewed changes
pkg/handler/enqueue_mapped.go
@@ -36,33 +34,41 @@ var _ EventHandler = &EnqueueRequestsFromMapFunc{}
//
// For UpdateEvents which contain both a new and old object, the transformation function is run on both
// objects and both sets of Requests are enqueue.
type EnqueueRequestsFromMapFunc struct {
func EnqueueRequestsFromMapFunc(mapFN func(MapObject) []reconcile.Request) EventHandler {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func EnqueueRequestsFromMapFunc(mapFN func(MapObject) []reconcile.Request) EventHandler {
func MapFunc(mapFN func(MapObject) []reconcile.Request) EventHandler {

Do we want to go for something a little simpler? wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

All existing mappers are called EnqueueRequestSomething I thought it would be nice to keep that pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

It also makes this change easier for existing users

@alvaroaleman alvaroaleman changed the title ✨ Add handler.FromMapFunc to simplify creation of custom mappers ✨ Simplify usage of EnqueueRequestsFromMapFunc Aug 6, 2020
@k8s-ci-robot
Copy link
Contributor

@alvaroaleman: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-controller-runtime-apidiff-master 0f6c191 link /test pull-controller-runtime-apidiff-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

vincepri
vincepri approved these changes Aug 6, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1095de6 into kubernetes-sigs:master Aug 6, 2020
@DirectXMan12
Copy link
Contributor

FYI, this is breaking

@timebertt timebertt mentioned this pull request Dec 22, 2020
Closed
dprince added a commit to dprince/osp-director-operator that referenced this pull request Apr 1, 2021
@dprince
Reconcile with controller-runtime v0.7.2 changes
b799da3 
This updates our controllers to work with recent
controller runtime updates (Simplify usage of
EnqueueRequestsFromMapFunc, and Remove MapObject type and use
client.Object directly):
 - kubernetes-sigs/controller-runtime#1119
 - kubernetes-sigs/controller-runtime#1207
dprince added a commit to dprince/osp-director-operator that referenced this pull request Apr 1, 2021
@dprince
Reconcile with controller-runtime v0.7.2 changes
75e6d28 
This updates our controllers to work with recent
controller runtime updates (Simplify usage of
EnqueueRequestsFromMapFunc, and Remove MapObject type and use
client.Object directly):
 - kubernetes-sigs/controller-runtime#1119
 - kubernetes-sigs/controller-runtime#1207
amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 4, 2021
@amisevsk
Update codebase to reflect later k8s APIs
92a7c72 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 10, 2021
@amisevsk
Update codebase to reflect later k8s APIs
ff66594 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
sleshchenko pushed a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 11, 2021
@amisevsk @sleshchenko
Update codebase to reflect later k8s APIs
119b8ac 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 12, 2021
@amisevsk
Update codebase to reflect later k8s APIs
ce78ec6 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 12, 2021
@amisevsk
Update codebase to reflect later k8s APIs
1b116dc 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
amisevsk added a commit to amisevsk/devworkspace-operator that referenced this pull request Aug 12, 2021
@amisevsk
Update codebase to reflect later k8s APIs
3b4926b 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
JPinkney pushed a commit to devfile/devworkspace-operator that referenced this pull request Aug 30, 2021
@amisevsk @JPinkney
Update codebase to reflect later k8s APIs
8555854 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
sleshchenko pushed a commit to devfile/devworkspace-operator that referenced this pull request Sep 2, 2021
@amisevsk @sleshchenko
Update codebase to reflect later k8s APIs
ae6de50 
Prep code for supporting k8s 1.21:

* Add context.Context parameter to Reconcile() functions [1]
* Add RBAC get/create/update coordination.k8s.io/v1 Leases (replacing a
  configmap for leader election locks) [2]
* Adapt event handler code to reflect simplification [3]
* Adapt to apimachinery/pkg/runtime Log deprecation (replaced with
  pkg/client Log) [4]
* client.Object is preferred in favor of runtime.Object (v0.7.0 release) [5]
* Use admission/v1 instead of v1beta1 for webhook requests [6]

[1] - kubernetes-sigs/controller-runtime#1054
[2] - kubernetes-sigs/controller-runtime#1144
[3] - kubernetes-sigs/controller-runtime#1119
[4] - kubernetes-sigs/controller-runtime#1105
[5] - kubernetes-sigs/controller-runtime#898
      kubernetes-sigs/controller-runtime#1118
[6] - kubernetes-sigs/controller-runtime#1284

kubernetes-sigs/controller-runtime@a32b29d
Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@droot droot Awaiting requested review from droot

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.x
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@alvaroaleman @k8s-ci-robot @vincepri @DirectXMan12