Prune API of the resolver package. #2330
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
It's a test stub that has only one consumer. Generating a fake into pkg/fakes with counterfeiter and using it for tests in pkg/controller/operators/olm introduces a number of transitive package dependencies that make it difficult to unwind the resolver package from other runtime components. Signed-off-by: Ben Luddy <[email protected]>
It has been living in the resolver package as a holdover from earlier generations of resolution, but is only consumed by olm-operator as part of OperatorGroup reconciliation. Signed-off-by: Ben Luddy <[email protected]>
The only user of the provided/required API labeling functionality is olm-operator, but it was still residing in the resolver package. Signed-off-by: Ben Luddy <[email protected]>
This interface is used only by olm-operator and doesn't provide a useful abstraction on top of directly reading fields from Operator structs. Signed-off-by: Ben Luddy <[email protected]>
openshift-ci
bot
added
the
approved
SourceQuerier is a holdover from the previous resolution implementation. Now, it only gets minor usage in catalog-operator. Unused methods have been removed, and SourceQuerier has moved to pkg/controller/operators/catalog beside where it is used. Signed-off-by: Ben Luddy <[email protected]>
benluddy
force-pushed
the
shrink-resolver-surface
branch
from
b4dc6df to
5ef4dab
Compare
dinhxuanvu
reviewed
Aug 18, 2021
njhale
approved these changes
Aug 18, 2021
| @@ -890,7 +890,7 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { | |||
| // get the set of sources that should be used for resolution and best-effort get their connections working | |||
| logger.Debug("resolving sources") | |||
|
|
|||
| querier := resolver.NewNamespaceSourceQuerier(o.sources.AsClients(o.namespace, namespace)) | |||
There was a problem hiding this comment.
TIL: this wasn't (never?) actually dependency injected
even if it was, the DI should have taken a factory anyway
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy, dinhxuanvu, 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The resolver package exports a number of identifiers that have not been used for resolution in over a year. As part of the ongoing effort to decouple purely-resolution-related concerns from the rest of OLM, some of these have been rehomed into the packages that still consume them, and entirely unused code has been removed.