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.
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.
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
Makes
OLMVariableSource
responsible for fetching #245Makes
OLMVariableSource
responsible for fetching #245Changes from all commits
365253c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the order of variables returned from
olmVariableSource.GetVariables
is not important here.Since it is now using a client to fetch operators - order is different now and tests were relying on order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like an important question to resolve at the public-facing level - if the order is not guaranteed, it should be noted in the API docs. If it is, a simple sort-in-place can be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not defining an API here - we are implementing a contract/interface required by Deppy. Solver will be calling
GetVariables
and as far as I see it doesn't require variable s to be sorted in a specific way.https://github.com/operator-framework/deppy/blob/9a7655cd364b3900d394f81a752731200e2d01a4/pkg/deppy/input/variable_source.go#L9-L12
IMO nether sorting variables nor documenting ordering on the operator-controller side is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to clarify: there are some places where the order is very important for the resolution process. For example, we want to select latest possible version for the solution.
But I think here it is not important. However thinking about debuggability - it still might be a good idea to have some stable order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was partially wrong here:
I looked into this a bit more. It looks like the fake client does not guarantee order (at least I do not see anything about this in docs), but de facto it does sorting (we hit this code eventually).
So in the test we add
prometheus
and thenpackageA
into the fake, but whenGetVariables
calls the client - it returnspackageA
first andprometheus
because the result list gets sorted.Our options here:
GetVariables
after calling the client. This is only required for making the test simplier. I think it is not right to introduce sorting in the code which we test just to make the test code a little bit simplier.GetVariables
.List
result. Not sure this has any benefit: we will have to introduce extra testing code to avoid changing existing testing code (assertions).Since we now know that fake client returns in deterministic order (not documented, but still) - I think there is little to no benefit in sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If sort order is immaterial except for testing, I agree that your third option is the best.