controller-runtime icon indicating copy to clipboard operation
controller-runtime copied to clipboard
verified publisher icon kubernetes-sigs

⚠ cache.BuilderWithOptions inherit options from caller

Open joelanford opened this issue 3 years ago • 1 comments

using cache.BuilderWithOptions does not properly inherit all options passed in from the caller:

  • scheme was overridden instead of merged
  • selectors were not inherited at all, even if specified options selectors remained unset
  • transforms were not inherited at all, even if specified options transforms remained unset
  • disable deep copy settings were not inherited at all, even if specified options for disabling deep copies remained unset

This commit resolves this issues by implementing merge logic for all fields in the cache.Options struct:

  • Schemes are merged
  • RESTMapper is chosen by precedence only falling back to inherited options if left unset in specified options
  • Resync is chosen by precedence only falling back to inherited options if left unset in specified options
  • Namespace is chosen by precedence only falling back to inherited options if left unset in specified options
  • Selectors are merged. If both inherited and specified Options defined selectors for a given type, those selectors are merged via logical AND.
  • DisableDeepCopy is combined via precedence. Only if a value for a particular GVK is unset in the specified options will a value from the inherited options be used.
  • Transform functions are combined via chaining. If both inherited and specified options define a transform function, the transform function from the inherited options will be called first, and the transform function from the specified options will be called second.

This is a breaking change if your code does all of the following:

  1. Uses cache.BuilderWithOptions
  2. Specifically depends on the fact that inherited options are not currently considered for selectors, disabling deep copies, and transform functions OR that the inherited scheme is ignored if it is defined in the options specified with cache.BuilderWithOptions.

Signed-off-by: Joe Lanford [email protected]

joelanford avatar Aug 16 '22 21:08 joelanford

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joelanford

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 avatar Aug 16 '22 21:08 k8s-ci-robot

/retest

joelanford avatar Aug 30 '22 21:08 joelanford

/retest

/lgtm

sbueringer avatar Aug 31 '22 10:08 sbueringer

/retest

joelanford avatar Aug 31 '22 11:08 joelanford

Oh wasn't aware self-approve is enabled in this repo. Sorry if this wasn't intended to be merged already

sbueringer avatar Aug 31 '22 12:08 sbueringer