Can we improve the design of the storage interface?

[joelanford]

In #292, we modified the storage interface from:

type Storage interface {
    Load(ctx context.Context, owner client.Object) ([]unstructured.Unstructured, error)
    Store(ctx context.Context, owner client.Object, objects []client.Object) error
}

to

type Storage interface {
    Load(ctx context.Context, owner client.Object) (fs.FS, error)
    Store(ctx context.Context, owner client.Object, bundle fs.FS) error
    Delete(ctx context.Context, owner client.Object) error
}

The two primary changes were:

• storing an fs.FS rather than a list of objects, thus making the storage interface more generic to support use cases outside of plain bundles (e.g. helm charts)
• adding a Delete method that enables callers to delete bundle contents when Bundle objects are removed from the cluster.

This was a good incremental step in improving the usefulness of the storage interface, but are there other tweaks or improvements to make?

For example, should we add a Reap(ctx context.Context, keeps []client.Object) error method that would enable the storage implementation to clear out unwanted bundle contents? If we added that method, we could revisit #309 and drop our use of finalizers. However certain implementations of Reap could be costly (both in dollars and in compute resources), so we should consider tradeoffs.

As another example, should we change the key from client.Object to string? If we did that, we could define Keys(context.Context) ([]string, error) which is more generic that Reap, and would allow reap mechanics to be implemented on top. However, a downside is that some Storage implementations might be able to make use of an entire client.Object (e.g. to add owner references to any on-cluster objects it might create). Perhaps we get around that by specifically defining the key as the bundle name, which would allow the storage implementation to Get the bundle if it needed more context?

[github-actions[bot]]

This issue has become stale because it has been open 60 days with no activity. The maintainers of this repo will remove this label during issue triage. Adding the lifecycle/frozen label will cause this issue to ignore lifecycle events.

[tylerslaton]

This is still relevant. The Reap() method sounds interesting and can help to cut down on undesired resources sticking around. The storage interface has changed slightly since the time of writing this issue so whoever is looking at this issue should take a look and consider what is still relevant here.

https://github.com/operator-framework/rukpak/blob/e4303270f33b3b88f1c97b04f49a6cf3042d158d/internal/storage/storage.go#L11-L26

[github-actions[bot]]

This issue has become stale because it has been open 60 days with no activity. The maintainers of this repo will remove this label during issue triage or it will be removed automatically after an update. Adding the lifecycle/frozen label will cause this issue to ignore lifecycle events.