-
Notifications
You must be signed in to change notification settings - Fork 67
🌱 Add internal direct bundle install support #2252
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
package resolve | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"io/fs" | ||
"reflect" | ||
|
||
bsemver "github.com/blang/semver/v4" | ||
|
||
"github.com/operator-framework/operator-registry/alpha/action" | ||
"github.com/operator-framework/operator-registry/alpha/declcfg" | ||
|
||
ocv1 "github.com/operator-framework/operator-controller/api/v1" | ||
"github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" | ||
"github.com/operator-framework/operator-controller/internal/shared/util/image" | ||
) | ||
|
||
const ( | ||
directBundleInstallImageAnnotation = "olm.operatorframework.io/bundle-image" | ||
) | ||
|
||
type BundleResolver struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about to name it |
||
ImagePuller image.Puller | ||
ImageCache image.Cache | ||
} | ||
|
||
func (r *BundleResolver) Resolve(ctx context.Context, ext *ocv1.ClusterExtension, _ *ocv1.BundleMetadata) (*declcfg.Bundle, *bsemver.Version, *declcfg.Deprecation, error) { | ||
if ext.Annotations == nil || ext.Annotations[directBundleInstallImageAnnotation] == "" { | ||
return nil, nil, nil, fmt.Errorf("ClusterExtension is missing required annotation %s", directBundleInstallImageAnnotation) | ||
} | ||
Comment on lines
+30
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is performed even in MultiResolver, before invoking BundleResolver. Hence, we can drop it here. |
||
bundleFS, canonicalRef, _, err := r.ImagePuller.Pull(ctx, ext.Name, ext.Annotations[directBundleInstallImageAnnotation], r.ImageCache) | ||
if err != nil { | ||
return nil, nil, nil, err | ||
} | ||
|
||
// TODO: This is a temporary workaround to get the bundle from the filesystem | ||
// until the operator-registry library is updated to support reading from a | ||
// fs.FS. This will be removed once the library is updated. | ||
bundlePath, err := getDirFSPath(bundleFS) | ||
Comment on lines
+38
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really don't like the FS interface, as it's so limiting, and you're already needing to reflect it to get the proper path. I'd almost prefer to get rid of the FS interface, and simply use a path string. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (This is really just me whining about the FS interface is all... not asking for a change unless you agree and really want to.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't disagree. But, atm, this FG is just an artifice to help us play around with the boxcutter status without having to deal with catalogs. Ultimately, I'd probably suggest moving away from FS and towards a formalized bundle interface that can surface all this kind of information without too many headaches |
||
if err != nil { | ||
panic(fmt.Errorf("expected to be able to recover bundle path from bundleFS: %v", err)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why panic instead of
|
||
} | ||
|
||
// Render the bundle | ||
render := action.Render{ | ||
Refs: []string{bundlePath}, | ||
AllowedRefMask: action.RefBundleDir, | ||
} | ||
fbc, err := render.Run(ctx) | ||
if err != nil { | ||
return nil, nil, nil, err | ||
} | ||
if len(fbc.Bundles) != 1 { | ||
return nil, nil, nil, errors.New("expected exactly one bundle") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps you can provide in the error message the number of bundles we got? |
||
} | ||
bundle := fbc.Bundles[0] | ||
bundle.Image = canonicalRef.String() | ||
v, err := bundleutil.GetVersion(bundle) | ||
if err != nil { | ||
return nil, nil, nil, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you provide the error context here? |
||
} | ||
return &bundle, v, nil, nil | ||
} | ||
|
||
// A function to recover the underlying path string from os.DirFS | ||
func getDirFSPath(f fs.FS) (string, error) { | ||
v := reflect.ValueOf(f) | ||
|
||
// Check if the underlying type is a string (its kind) | ||
if v.Kind() != reflect.String { | ||
return "", fmt.Errorf("underlying type is not a string, it is %s", v.Kind()) | ||
} | ||
|
||
// The type itself (os.dirFS) is unexported, but its Kind is a string. | ||
// We can convert the reflect.Value back to a regular string using .Interface() | ||
// after converting it to a basic string type. | ||
path, ok := v.Convert(reflect.TypeOf("")).Interface().(string) | ||
if !ok { | ||
return "", fmt.Errorf("could not convert reflected value to string") | ||
} | ||
|
||
return path, nil | ||
} |
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 want to reflect in the annotation name that it is alpha feature, so something like: