馃尡 Part 4: Reduce number of variable sources. Required packages #500
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,99 +1,64 @@ | ||
| package variablesources | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "sort" | ||
|
|
||
| mmsemver "github.com/Masterminds/semver/v3" | ||
|
|
||
| operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
| "github.com/operator-framework/operator-controller/internal/catalogmetadata" | ||
| catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter" | ||
| catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort" | ||
| olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables" | ||
| ) | ||
|
|
||
| // MakeRequiredPackageVariables returns a variable which represent | ||
| // explicit requirement for a package from an user. | ||
| // This is when an user explicitly asks "install this" via Operator API. | ||
| func MakeRequiredPackageVariables(allBundles []*catalogmetadata.Bundle, operators []operatorsv1alpha1.Operator) ([]*olmvariables.RequiredPackageVariable, error) { | ||
| result := make([]*olmvariables.RequiredPackageVariable, 0, len(operators)) | ||
|
|
||
| for _, operator := range operators { | ||
| packageName := operator.Spec.PackageName | ||
| channelName := operator.Spec.Channel | ||
| versionRange := operator.Spec.Version | ||
|
|
||
| predicates := []catalogfilter.Predicate[catalogmetadata.Bundle]{ | ||
| catalogfilter.WithPackageName(packageName), | ||
| } | ||
|
|
||
| if channelName != "" { | ||
| predicates = append(predicates, catalogfilter.InChannel(channelName)) | ||
| } | ||
|
|
||
| if versionRange != "" { | ||
| vr, err := mmsemver.NewConstraint(versionRange) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("invalid version range %q: %w", versionRange, err) | ||
| } | ||
| predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr)) | ||
| } | ||
|
|
||
| resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(predicates...)) | ||
| if len(resultSet) == 0 { | ||
| if versionRange != "" && channelName != "" { | ||
| return nil, fmt.Errorf("no package %q matching version %q found in channel %q", packageName, versionRange, channelName) | ||
| } | ||
| if versionRange != "" { | ||
| return nil, fmt.Errorf("no package %q matching version %q found", packageName, versionRange) | ||
| } | ||
| if channelName != "" { | ||
| return nil, fmt.Errorf("no package %q found in channel %q", packageName, channelName) | ||
| } | ||
|
Comment on lines
+45
to
+53
There was a problem hiding this comment. What do you think about building up the error message as we're building the predicates (since we're already making the necessary conditional checks as we go)? There was a problem hiding this comment. I think I'm not in favour of this approach because:
There was a problem hiding this comment. I agree with the emphasis on readability here. |
||
| return nil, fmt.Errorf("no package %q found", packageName) | ||
| } | ||
| sort.SliceStable(resultSet, func(i, j int) bool { | ||
| return catalogsort.ByVersion(resultSet[i], resultSet[j]) | ||
| }) | ||
|
|
||
| result = append(result, olmvariables.NewRequiredPackageVariable(packageName, resultSet)) | ||
| } | ||
|
|
||
| return result, nil | ||
| } | ||
Oops, something went wrong.
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.
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.
Since I had similar comments on previous parts of this refactoring & most of our errors use double quotation marks I updated the format here.
I'm happy to revert this and move into a separate PR if this is too distracting
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.
+1 on keeping this