-
Notifications
You must be signed in to change notification settings - Fork 50
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
🌱 Migrate Rukpak #1032
🌱 Migrate Rukpak #1032
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -0,0 +1,244 @@ | |||
/* |
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.
Commenting here to call specific attention to this file. How much more of the k8s stuff here can be removed? Do we need the json:"..."
tags? Are we OK with the fact that I copied an autogenerated func into this file in order to maintain DeepCopy()
?
General comment: there are likely some things copied over that are unused. I'll go through and make sure that's all removed. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1032 +/- ##
==========================================
- Coverage 77.00% 73.46% -3.55%
==========================================
Files 19 32 +13
Lines 1357 1986 +629
==========================================
+ Hits 1045 1459 +414
- Misses 222 368 +146
- Partials 90 159 +69
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ecacb47
to
58cd0bf
Compare
I've removed everything I could find that was unused; see comparison here to see what I left out. |
WDYT about dumping everything in under I'm very much in favor of moving our rukpak code directly into this repo, but I'm also feeling a little bit like we need to (in a future PR) step back and take stock of our suddenly sprawling codebase to see if there are obsolete abstractions (e.g. unpack + storage) and other ways to reduce our package count and tidy things up a bit. |
@@ -280,7 +280,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp | |||
case rukpaksource.StateUnpacked: | |||
// TODO: Add finalizer to clean the stored bundles, after https://github.com/operator-framework/rukpak/pull/897 | |||
// merges. | |||
if err := r.Storage.Store(ctx, ext, unpackResult.Bundle); err != nil { | |||
if err := r.Storage.Store(ctx, ext.GetName(), unpackResult.Bundle); err != 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.
Was this just because you realized we only need to store the name and not the whole struct?
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.
Yes, the thing we're actually storing is the unpackResult.Bundle
, and ext
was passed in purely for its identifier.
internal/handler/interfaces.go
Outdated
bd "github.com/operator-framework/operator-controller/internal/bundledeployment" | ||
) | ||
|
||
type Handler interface { |
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.
Should this maybe become less generic, so RegistryHandler
perhaps?
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.
Yes, there's a good argument for that since we're only using one of the 3 types from rukpak.
internal/provisioner/plain/plain.go
Outdated
manifestsDir = "manifests" | ||
) | ||
|
||
func HandleBundleDeployment(_ context.Context, fsys fs.FS, _ *bundledeployment.BundleDeployment) (*chart.Chart, chartutil.Values, error) { |
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.
Seems like a unit test on this with Bundle in/Chart out would be good. Just in case Helm bumps, etc, shift something on us
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.
Right; and that brings up another larger issue for this PR: Rukpak was kind of infamous for testing with e2e tests instead of unit tests, so we can't bring over a lot of those tests as-is. Personally I think there's probably too much new code in this PR for adding unit tests for all of it and I'd prefer to open a new issue for that if you think that's reasonable.
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.
sounds good.to me. just need an issue for adding rukpakian related unit tests, to do 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.
Created #1037
internal/source/image_registry.go
Outdated
"path/filepath" | ||
"strings" | ||
|
||
"github.com/containerd/containerd/archive" |
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.
The focus here is just pull in the code, but maybe we should add an issue to make more use of containerd
for the future? Seems like containerd
has image operations that could simplify TLS and authentication handling, streamline pulling images, and optimize unpacking layers.
I'm very much in agreement with this and your other thought as well. Since I filtered out a lot of the types there's not currently much reason for the abstract types like For now at the very least I'll move everything into the |
This is what I originally did for the original integration (before we pulled everything back) |
We might possibly want to remove rukpak references from |
Looks as though a lot of the original code was not pulled over (good), and necessary changes made to only support imagev1. |
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.
/lgtm
@dtfranz, you'll need to rebase due to go.mod changes.... |
Moves rukpak code imported by operator-controller into the internal/ folder. Signed-off-by: dtfranz <dfranz@redhat.com>
/lgtm |
e5470e2
Moves rukpak code imported by operator-controller into the internal/ folder. Signed-off-by: dtfranz <dfranz@redhat.com>
Moves rukpak code imported by operator-controller into the internal/ folder. Signed-off-by: dtfranz <dfranz@redhat.com>
Moves rukpak code imported by operator-controller into the
internal/
folder.Fixes #997