Uh oh!
There was an error while loading. Please reload this page.
Add annotations to BundleDeployment#442
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -5,6 +5,7 @@ import ( | ||
| "fmt" | ||
| "sort" | ||
| mmsemver "github.com/Masterminds/semver/v3" | ||
| "github.com/operator-framework/deppy/pkg/deppy" | ||
| "github.com/operator-framework/deppy/pkg/deppy/input" | ||
| @@ -19,7 +20,9 @@ var _ input.VariableSource = &InstalledPackageVariableSource{} | ||
| type InstalledPackageVariableSource struct { | ||
| catalogClient BundleProvider | ||
| successors successorsFunc | ||
| pkgName string | ||
| bundleName string | ||
| bundleVersion string | ||
| } | ||
| func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]deppy.Variable, error) { | ||
| @@ -28,18 +31,23 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de | ||
| return nil, err | ||
| } | ||
| vr, err := mmsemver.NewConstraint(r.bundleVersion) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // find corresponding bundle for the installed content | ||
| resultSet := catalogfilter.Filter(allBundles, catalogfilter.And( | ||
| catalogfilter.WithPackageName(r.pkgName), | ||
| catalogfilter.WithName(r.bundleName), | ||
| catalogfilter.InMastermindsSemverRange(vr), | ||
| )) | ||
| if len(resultSet) == 0 { | ||
| return nil, fmt.Errorf("bundle for package %q with name %q at version %q not found", r.pkgName, r.bundleName, r.bundleVersion) | ||
| } | ||
| if len(resultSet) > 1 { | ||
| return nil, fmt.Errorf("more than one bundle for package %q with name %q at version %q found", r.pkgName, r.bundleName, r.bundleVersion) | ||
| } | ||
Comment on lines
-37
to
-39
MemberAuthor There was a problem hiding this comment. I believe this is no longer relevant given that we filter by package name, bundle name and bundle version. | ||
Comment on lines
-40
to
-42
MemberAuthor There was a problem hiding this comment. I think we no longer need sorting by version since we filter bundles by bundle version amongst other things. I assume previously we were doing sorting beucase multiple bundles could in theory use the same image (.e.g some sort of fake version). Member There was a problem hiding this comment. Sorting, I think, was to make sure the resolver favored high semver versions over low semver versions. Maybe different context though? MemberAuthor There was a problem hiding this comment. For upgrade edges there is a separate sorting below in this file (not in the diff) which is still needed. This piece of code is where we look for a currently installed bundle by image ref. I suspect that there may be multiple bundles with the same image, but different versions 🤷♂️. So in the current implementation we need sorting to prefer higher version. But when filtering bundles by of package name, bundle name and bundle version we should be able to uniquely identify a bundle. So I expect there always will be 1 or 0 bundles. @jmprusi might be able to give some more context Member There was a problem hiding this comment. Got it. It looks like we check the result set for 0 bundles, but not more than 1. Can we add another check that errors if there is more than 1 bundle? | ||
| installedBundle := resultSet[0] | ||
| upgradeEdges, err := r.successors(allBundles, installedBundle) | ||
| @@ -54,16 +62,14 @@ func (r *InstalledPackageVariableSource) GetVariables(ctx context.Context) ([]de | ||
| }, nil | ||
| } | ||
| func NewInstalledPackageVariableSource(catalogClient BundleProvider, pkgName, bundleName, bundleVersion string) *InstalledPackageVariableSource { | ||
| return &InstalledPackageVariableSource{ | ||
| catalogClient: catalogClient, | ||
| successors: legacySemanticsSuccessors, | ||
| pkgName: pkgName, | ||
| bundleName: bundleName, | ||
| bundleVersion: bundleVersion, | ||
| } | ||
| } | ||
| // successorsFunc must return successors of a currently installed bundle | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Do we need to care about bundle deployments created directly? These annotations will only be present on bundle deployments reconciled by operator-controller.
Edit: I think we do need to care so this whole thing might not fly.
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.
For now, I don't think we need to care, at least in this context.
I sorta think this context is: is there a BD for this operator already? (And maybe that means we should have a label on the BD like
operators.operatorframework.io/operator-namewhose value isOperator.metadata.name?Perhaps in the future, there's a separate feature that helps the resolver become aware of other BDs, but I don't think we need to do that now.