Remove cache.OperatorCacheProvider interface. #2680
Conversation
This interface is unused, so it can be removed as a step toward a stable and supported cache package API. Signed-off-by: Ben Luddy <bluddy@redhat.com>
openshift-ci
bot
added
the
approved
|
/lgtm |
| @@ -29,7 +29,7 @@ type constraintProvider interface { | |||
| } | |||
|
|
|||
| type Resolver struct { | |||
| cache cache.OperatorCacheProvider | |||
| cache *cache.Cache | |||
There was a problem hiding this comment.
Would OperatorCacheProvider be useful for injecting mocks? or do you imagine further near-term refactoring to invalidate that work?
There was a problem hiding this comment.
Even if it were, I'd expect the interface to be defined by the consumer instead of exported from the cache package. I'm also not convinced that there will be a need to mock the cache. The contents of a cache are injectable, so tests are able to configure real objects that return what they want.
There was a problem hiding this comment.
Even if it were, I'd expect the interface to be defined by the consumer instead of exported from the cache package.
I share that perspective. It should probably be defined in the resolver package (or even a Cache interface with the method signatures the resolver cares about).
The contents of a cache are injectable, so tests are able to configure real objects that return what they want.
My rule of thumb is to prefer abstracting with an interface (at the consumer, of course). I find that it helps to decouple the consumer tests from a particular implementation for two reasons:
- It's usually easier to generate specific call outcomes without wrestling a real implementation to do so; for example, errors/bad data, a specific sequence occurring with a non-deterministic operation (e.g. map-key iteration), etc
- Test-fixtures are usually smaller and more manageable
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, njhale 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 |
This interface is unused, so it can be removed as a step toward a
stable and supported cache package API.