Skip to content

Remove cache.OperatorCacheProvider interface. #2680

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

Merged
merged 1 commit into from
Mar 2, 2022
Merged

Remove cache.OperatorCacheProvider interface. #2680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions pkg/controller/registry/resolver/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ type Cache struct {
m sync.RWMutex
}

var _ OperatorCacheProvider = &Cache{}

type Option func(*Cache)

func WithLogger(logger logrus.StdLogger) Option {
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/registry/resolver/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ type constraintProvider interface {
}

type Resolver struct {
cache cache.OperatorCacheProvider
cache *cache.Cache
Copy link
Member

Choose a reason for hiding this comment

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

Would OperatorCacheProvider be useful for injecting mocks? or do you imagine further near-term refactoring to invalidate that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

log logrus.FieldLogger
pc *predicateConverter
systemConstraintsProvider constraintProvider
Expand Down