Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default #437

danehans · 2020-08-05T23:41:07Z

kubernetes-sigs/controller-runtime#554 merged to fix kubernetes-sigs/controller-runtime#321. This PR updates the operator to use the controller-runtime DynamicRestMapper (default option) instead of using the internal implementation. The controller-runtime DynamicRestMapper was tested by:

Running the ingress operator locally.
Removing the ingresscontroller CRD and default ingresscontroller CR from the api server.
Recreating the the ingresscontroller CRD and the operator successfully created the default ingresscontroller CR.

/assign @Miciah @knobunc
/cc @frobware @sgreene570

openshift-ci-robot · 2020-08-05T23:41:10Z

@danehans: This pull request references Bugzilla bug 1866568, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "4.5.z" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1866568: Updates RestMapper to use controller-runtime DynamicRestMapper

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.

danehans · 2020-08-05T23:42:51Z

/bugzilla refresh

openshift-ci-robot · 2020-08-05T23:42:57Z

@danehans: This pull request references Bugzilla bug 1866568, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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.

Miciah · 2020-08-06T00:13:27Z

This is great! Can we simplify further by deleting pkg/util/restmapper.go and using apiutil.NewDynamicRESTMapper directly in pkg/operator/operator.go?

danehans · 2020-08-12T23:12:15Z

This is great! Can we simplify further by deleting pkg/util/restmapper.go and using apiutil.NewDynamicRESTMapper directly in pkg/operator/operator.go?

@Miciah done in commit 826c995.

Miciah · 2020-08-12T23:29:47Z

This is great! Can we simplify further by deleting pkg/util/restmapper.go and using apiutil.NewDynamicRESTMapper directly in pkg/operator/operator.go?

@Miciah done in commit 826c995.

Thanks! I was hoping we could collapse it down to the following:

		MapperProvider: apiutil.NewDynamicRESTMapper,

But it looks like apiutil.NewDynamicRESTMapper's variadic opts parameter causes a type mismatch (cannot use apiutil.NewDynamicRESTMapper (value of type func(cfg *rest.Config, opts ...apiutil.DynamicRESTMapperOption) (meta.RESTMapper, error)) as func(c *rest.Config) (meta.RESTMapper, error) value in struct literal).

However, it looks like the dynamic REST mapper is now the default mapper:

cluster-ingress-operator/vendor/sigs.k8s.io/controller-runtime/pkg/manager/manager.go

Lines 384 to 388 in 8aa1ce2

    
           if options.MapperProvider == nil { 
        
           	options.MapperProvider = func(c *rest.Config) (meta.RESTMapper, error) { 
        
           		return apiutil.NewDynamicRESTMapper(c) 
        
           	} 
        
           }

Any reason not to delete the designated initializer for the MapperProvider field entirely so we get the default?

openshift-ci-robot · 2020-08-13T18:22:40Z

@danehans: This pull request references Bugzilla bug 1866568, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default

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.

danehans · 2020-08-13T18:22:45Z

Any reason not to delete the designated initializer for the MapperProvider field entirely so we get the default?

@Miciah I wasn't aware that the DynamicRestMapper is now the default. Thanks for looking into this. Commit 179dcbc implements your feedback.

Miciah · 2020-08-13T19:50:23Z

I love seeing all that code deleted!
/lgtm
Can we do the same cleanup in cluster-dns-operator? It's using controller-runtime 0.6.0, which appears to have the fix.

openshift-ci-robot · 2020-08-13T19:50:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans, Miciah

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:

~~OWNERS~~ [Miciah,danehans]

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

openshift-bot · 2020-08-13T19:53:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

danehans · 2020-08-13T21:32:43Z

@Miciah thanks for making me aware that dns operator uses the same internal RestMapper implementation. openshift/cluster-dns-operator#189 is the same PR as this for dns operator.

danehans · 2020-08-13T21:35:33Z

/bin/bash: line 129: /tmp/artifacts/junit/junit_install_status.xml: No such file or directory
error: failed to execute wrapped command: exit status 1

^ is unrelated to this PR.

/test e2e-aws

openshift-bot · 2020-08-13T21:37:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-13T22:55:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T06:10:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T07:15:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T08:33:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T09:12:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T09:25:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T10:56:31Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T11:09:31Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T13:06:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T14:11:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T15:55:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T17:13:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T17:52:31Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T19:10:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T19:49:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T20:02:29Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T21:20:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T21:46:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T23:17:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-16T23:43:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T04:55:33Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T05:21:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T08:36:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T08:49:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T12:56:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T16:11:06Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T18:34:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T22:01:31Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-17T23:19:32Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-08-18T00:40:59Z

@danehans: All pull requests linked via external trackers have merged: openshift/cluster-ingress-operator#437. Bugzilla bug 1866568 has been moved to the MODIFIED state.

In response to this:

Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default

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.

openshift-ci-robot assigned knobunc and Miciah Aug 5, 2020

openshift-ci-robot requested review from frobware and sgreene570 August 5, 2020 23:41

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Aug 5, 2020

openshift-ci-robot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 5, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 5, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Aug 5, 2020

danehans force-pushed the bz_1866568 branch from 50a2e0a to 826c995 Compare August 12, 2020 23:11

Removes DynamicRestMapper initialization since it's now the default

179dcbc

danehans force-pushed the bz_1866568 branch from 826c995 to 179dcbc Compare August 13, 2020 18:20

danehans changed the title ~~Bug 1866568: Updates RestMapper to use controller-runtime DynamicRestMapper~~ Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default Aug 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

danehans mentioned this pull request Aug 13, 2020

Bug 1868816: Removes RestMapper in favor of pkg defaults openshift/cluster-dns-operator#189

Merged

openshift-merge-robot merged commit 4c59487 into openshift:master Aug 18, 2020

Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default #437

Bug 1866568: Removes RestMapper since DynamicRestMapper is now the pkg default #437

Conversation

danehans commented Aug 5, 2020 • edited Loading

openshift-ci-robot commented Aug 5, 2020

danehans commented Aug 5, 2020

openshift-ci-robot commented Aug 5, 2020

Miciah commented Aug 6, 2020 • edited Loading

danehans commented Aug 12, 2020

Miciah commented Aug 12, 2020

openshift-ci-robot commented Aug 13, 2020

danehans commented Aug 13, 2020

Miciah commented Aug 13, 2020

openshift-ci-robot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

danehans commented Aug 13, 2020

danehans commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 13, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 16, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-bot commented Aug 17, 2020

openshift-ci-robot commented Aug 18, 2020

danehans commented Aug 5, 2020 •

edited

Loading

Miciah commented Aug 6, 2020 •

edited

Loading