✨ Starting Helm POC - pull in some rukpak #756

tmshort · 2024-04-16T15:55:31Z

Description

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

Signed-off-by: Todd Short <[email protected]>

varshaprasad96 · 2024-04-16T16:56:03Z

internal/controllers/clusterextension_controller.go

@@ -221,6 +228,15 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad
 	}
 }

+func (r *ClusterExtensionReconciler) GenerateExpectedBundleSource(o ocv1alpha1.ClusterExtension, bundlePath string) *rukpakapi.BundleSource {
+	return &rukpakapi.BundleSource{


The BundleSource here is stateless, and does not store any information with respect to the status.
Which means we need not create a separate API, instead just create a struct and pass it to the unpacker. This way, we can remove all the deep copy methods and not consider the bundle source as a deep copy object.

So, this API is not a k8s API, it's just a struct as you point out. It was convenient to use. However, it's named rukpakapi since it's part of the interface to the pulled-in rukpak code.

It had deepycopy methods, so thought this was intended to be a separate API. The naming is all good 👍

varshaprasad96 · 2024-04-16T17:05:26Z

internal/rukpak/api/bundle_types.go

+	// Type defines the kind of Bundle content being sourced.
+	Type SourceType `json:"type"`
+	// Image is the bundle image that backs the content of this bundle.
+	Image *ImageSource `json:"image,omitempty"`


For now we can consider only the Image source. The less code the better and once we have the prototype ready with parts of Helm working as a applier, we can work on expanding this to other sources. Wdyt?

varshaprasad96 · 2024-04-16T17:06:25Z

internal/rukpak/api/bundle_types.go

+	InsecureSkipVerify bool `json:"insecureSkipVerify,omitempty"`
+}
+
+type ProvisionerID string


We don't need Provisioners any more, since we will have just one controller handling unpacking, and installing for all kinds of bundles. Also, for now, we are going to consider only registryV1 formats.

varshaprasad96 · 2024-04-16T17:07:32Z

internal/rukpak/api/zz_generated.deepcopy.go

+	return out
+}
+
+// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.


We need not have BundleSource as a separate API which means as mentioned in the previous comment we can remove these deep copy methods.

varshaprasad96 · 2024-04-16T17:12:47Z

internal/rukpak/provisioner/helm/helm.go

+	ProvisionerID = "core-rukpak-io-helm"
+)
+
+func HandleBundleDeployment(_ context.Context, fsys fs.FS, bd *rukpakv1alpha2.BundleDeployment) (*chart.Chart, chartutil.Values, error) {


This bit of code is going to change since we don't have BundleDeployment anymore. Plus looks like this is using helm based bundles (from the provisionerID). We need to port the registryV1 piece of implementation, which would be:

Converting registryV1 to plain bundles: the conversion code which is called here: https://github.com/operator-framework/rukpak/blob/882714a00b5d99fdfc1b65992206c4e90edfd696/internal/provisioner/registry/registry.go#L22

The logic where we create a chart from the manifest objects: https://github.com/operator-framework/rukpak/blob/882714a00b5d99fdfc1b65992206c4e90edfd696/internal/provisioner/plain/plain.go#L28

This would need some amount of refinement in terms of creating the chart from unpacked bundle contents from the file system. Can we remove this from this PR, and work on this separately along with the helm install logic?

Yes, this code was just pulled in, it hasn't been updated. I expect that things will be iterated and removed.

varshaprasad96 · 2024-04-16T17:14:34Z

internal/rukpak/provisioner/plain/plain.go

+	manifestsDir = "manifests"
+)
+
+func HandleBundleDeployment(_ context.Context, fsys fs.FS, _ *rukpakv1alpha2.BundleDeployment) (*chart.Chart, chartutil.Values, error) {


As mentioned in the previous comment, this is going to change as we no longer have BundleDeployment. What do you think about removing the Handler implementations from this PR and working on them in follow ups?

varshaprasad96 · 2024-04-16T17:15:30Z

internal/rukpak/provisioner/registry/registry.go

+	ProvisionerID = "core-rukpak-io-registry"
+)
+
+func HandleBundleDeployment(ctx context.Context, fsys fs.FS, bd *rukpakv1alpha2.BundleDeployment) (*chart.Chart, chartutil.Values, error) {


The same as before in terms of removing BundleDeployment related handlers.

varshaprasad96 · 2024-04-16T17:17:02Z

internal/rukpak/source/configmaps.go

@@ -0,0 +1,85 @@
+package source
+
+import (


We can have just the internal/rukpak/source/image.go for now, so that we figure out the unpacker for just one source for now. All the other implementations in internal/rukpak/source/ can be removed for now

varshaprasad96 · 2024-04-16T17:18:53Z

internal/rukpak/source/unpacker.go

+	// For example, resolved image sources should reference a container image
+	// digest rather than an image tag, and git sources should reference a
+	// commit hash rather than a branch or tag.
+	ResolvedSource *rukpakapi.BundleSource


I'm wondering if we need the ResolvedSource here? What if we directly populate the ClusterExtension status with the details of resolved bundle?

That could be a follow on.

Also, I was trying to avoid changing k8s APIs

varshaprasad96 · 2024-04-16T17:20:07Z

internal/rukpak/util/util.go

+	ErrMaxGeneratedLimit = errors.New("reached the maximum generated Bundle limit")
+)
+
+func BundleDeploymentProvisionerFilter(provisionerClassName string) predicate.Predicate {


We are not using Provisioners here, so I think all the methods in this file can be removed and would not be useful.

tmshort · 2024-04-16T17:28:14Z

General note: it was just easy to pull in more code than necessary, rather than trying to figure out exactly what was needed.

tmshort · 2024-04-16T18:11:54Z

There's still quite possibly too much code in here.

varshaprasad96

Just a nit, the other changes look good!

Are you going to plug in the unpacker in the reconciler in this PR, or as a follow up?

varshaprasad96 · 2024-04-16T18:37:31Z

internal/rukpak/api/bundle_types.go

+	SourceTypeGit        SourceType = "git"
+	SourceTypeConfigMaps SourceType = "configMaps"
+	SourceTypeHTTP       SourceType = "http"


Nit: cleanups

Suggested change

SourceTypeGit SourceType = "git"

SourceTypeConfigMaps SourceType = "configMaps"

SourceTypeHTTP SourceType = "http"

varshaprasad96 · 2024-04-16T18:39:29Z

internal/rukpak/source/unpacker.go

+			MinVersion: tls.VersionTLS12,
+		}
+	}
+	httpTransport.TLSClientConfig.RootCAs = rootCAs


I think we don't need the TLS client config or the rootCAs. IIRC it was used only in http source.

Hmmm... even in the original code, it doesn't appear used. It's a clone (copy?) and isn't even referenced after this. Odd...

varshaprasad96 · 2024-04-16T18:41:19Z

internal/rukpak/util/hash.go

+// DeepHashObject writes specified object to hash using the spew library
+// which follows pointers and prints actual values of the nested objects
+// ensuring the hash does not change when a pointer changes.
+func DeepHashObject(obj interface{}) (string, error) {


TODO (follow up): We can have this for now, but just wondering if we need to ever use it anywhere to hash the entire spec of the object. The linter would complain in the future anyway, we can take care as the design proceeds.

tmshort · 2024-04-16T18:56:23Z

Are you going to plug in the unpacker in the reconciler in this PR, or as a follow up?

Follow up.

Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]>

varshaprasad96

The CI may probably fail, since this is a PoC not going to worry about that.

/lgtm
/approve

openshift-ci · 2024-04-16T19:02:30Z

New changes are detected. LGTM label has been removed.

* Fix suite_test.go Signed-off-by: Todd Short <[email protected]> * Copy over some rukpak code and replace BundleDeployment Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]> * fixup! Copy over some rukpak code and replace BundleDeployment --------- Signed-off-by: Todd Short <[email protected]>

plug in resolver Deal with removal of HigherBundleVersion Removed in e079129 Signed-off-by: Todd Short <[email protected]> :sparkles: Starting Helm POC - pull in some rukpak (#756) * Fix suite_test.go Signed-off-by: Todd Short <[email protected]> * Copy over some rukpak code and replace BundleDeployment Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]> * fixup! Copy over some rukpak code and replace BundleDeployment --------- Signed-off-by: Todd Short <[email protected]> Plugin unpacker, add Handler (#757) Signed-off-by: Varsha Prasad Narsing <[email protected]> Co-authored-by: [email protected] <[email protected]> A bit of cleanup (#761) Signed-off-by: Todd Short <[email protected]> :warning: Install the helm chart (#762) * Install the helm chart Signed-off-by: Todd Short <[email protected]> * fixup! Install the helm chart Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> Set up right watches and all labels to postrenderer (#763) Co-authored-by: [email protected] <[email protected]> :warning: Update owner keys (#765) * Update owner keys Signed-off-by: Todd Short <[email protected]> * fixup! Update owner keys Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> No more panics (#767) Signed-off-by: Todd Short <[email protected]> Add relevant RBAC to enable controller to watch resources (#776) Co-authored-by: [email protected] <[email protected]> Changes required for ClusterExtension to install an operator (#789) * Add relevant RBAC to enable controller to watch resources * Debugging iteration - one * ClusterExtension installing the operator - working --------- Co-authored-by: [email protected] <[email protected]> Fix some lints (#793) Signed-off-by: Todd Short <[email protected]> :sparkles: Getting cluster extension running (#795) * Getting cluster extension running * Specify namespace to create secret Set resolved and installed versions (#806) Remove install references to rukpak (#805) Signed-off-by: Todd Short <[email protected]> Consolidate error message generation (#807) Signed-off-by: Todd Short <[email protected]> Add make kind-redeploy (#808) Signed-off-by: Todd Short <[email protected]> Use rukpak as a library (#821) Signed-off-by: Todd Short <[email protected]> Improve caching and fix constant reconciles (#825) Improve performance by caching objects that only have ClusterExtension as owners. Signed-off-by: Varsha Prasad Narsing <[email protected]> Fix linter (#826) Signed-off-by: Varsha Prasad Narsing <[email protected]> Move helm-operator-plugin (#828) Signed-off-by: Todd Short <[email protected]> Really fix linter (#833) Signed-off-by: Todd Short <[email protected]>

plug in resolver Deal with removal of HigherBundleVersion Removed in e079129 Signed-off-by: Todd Short <[email protected]> :sparkles: Starting Helm POC - pull in some rukpak (operator-framework#756) * Fix suite_test.go Signed-off-by: Todd Short <[email protected]> * Copy over some rukpak code and replace BundleDeployment Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]> * fixup! Copy over some rukpak code and replace BundleDeployment --------- Signed-off-by: Todd Short <[email protected]> Plugin unpacker, add Handler (operator-framework#757) Signed-off-by: Varsha Prasad Narsing <[email protected]> Co-authored-by: [email protected] <[email protected]> A bit of cleanup (operator-framework#761) Signed-off-by: Todd Short <[email protected]> :warning: Install the helm chart (operator-framework#762) * Install the helm chart Signed-off-by: Todd Short <[email protected]> * fixup! Install the helm chart Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> Set up right watches and all labels to postrenderer (operator-framework#763) Co-authored-by: [email protected] <[email protected]> :warning: Update owner keys (operator-framework#765) * Update owner keys Signed-off-by: Todd Short <[email protected]> * fixup! Update owner keys Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> No more panics (operator-framework#767) Signed-off-by: Todd Short <[email protected]> Add relevant RBAC to enable controller to watch resources (operator-framework#776) Co-authored-by: [email protected] <[email protected]> Changes required for ClusterExtension to install an operator (operator-framework#789) * Add relevant RBAC to enable controller to watch resources * Debugging iteration - one * ClusterExtension installing the operator - working --------- Co-authored-by: [email protected] <[email protected]> Fix some lints (operator-framework#793) Signed-off-by: Todd Short <[email protected]> :sparkles: Getting cluster extension running (operator-framework#795) * Getting cluster extension running * Specify namespace to create secret Set resolved and installed versions (operator-framework#806) Remove install references to rukpak (operator-framework#805) Signed-off-by: Todd Short <[email protected]> Consolidate error message generation (operator-framework#807) Signed-off-by: Todd Short <[email protected]> Add make kind-redeploy (operator-framework#808) Signed-off-by: Todd Short <[email protected]> Use rukpak as a library (operator-framework#821) Signed-off-by: Todd Short <[email protected]> Improve caching and fix constant reconciles (operator-framework#825) Improve performance by caching objects that only have ClusterExtension as owners. Signed-off-by: Varsha Prasad Narsing <[email protected]> Fix linter (operator-framework#826) Signed-off-by: Varsha Prasad Narsing <[email protected]> Move helm-operator-plugin (operator-framework#828) Signed-off-by: Todd Short <[email protected]> Really fix linter (operator-framework#833) Signed-off-by: Todd Short <[email protected]>

plug in resolver Deal with removal of HigherBundleVersion Removed in e079129 Signed-off-by: Todd Short <[email protected]> :sparkles: Starting Helm POC - pull in some rukpak (#756) * Fix suite_test.go Signed-off-by: Todd Short <[email protected]> * Copy over some rukpak code and replace BundleDeployment Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]> * fixup! Copy over some rukpak code and replace BundleDeployment --------- Signed-off-by: Todd Short <[email protected]> Plugin unpacker, add Handler (#757) Signed-off-by: Varsha Prasad Narsing <[email protected]> Co-authored-by: [email protected] <[email protected]> A bit of cleanup (#761) Signed-off-by: Todd Short <[email protected]> :warning: Install the helm chart (#762) * Install the helm chart Signed-off-by: Todd Short <[email protected]> * fixup! Install the helm chart Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> Set up right watches and all labels to postrenderer (#763) Co-authored-by: [email protected] <[email protected]> :warning: Update owner keys (#765) * Update owner keys Signed-off-by: Todd Short <[email protected]> * fixup! Update owner keys Signed-off-by: Todd Short <[email protected]> --------- Signed-off-by: Todd Short <[email protected]> No more panics (#767) Signed-off-by: Todd Short <[email protected]> Add relevant RBAC to enable controller to watch resources (#776) Co-authored-by: [email protected] <[email protected]> Changes required for ClusterExtension to install an operator (#789) * Add relevant RBAC to enable controller to watch resources * Debugging iteration - one * ClusterExtension installing the operator - working --------- Co-authored-by: [email protected] <[email protected]> Fix some lints (#793) Signed-off-by: Todd Short <[email protected]> :sparkles: Getting cluster extension running (#795) * Getting cluster extension running * Specify namespace to create secret Set resolved and installed versions (#806) Remove install references to rukpak (#805) Signed-off-by: Todd Short <[email protected]> Consolidate error message generation (#807) Signed-off-by: Todd Short <[email protected]> Add make kind-redeploy (#808) Signed-off-by: Todd Short <[email protected]> Use rukpak as a library (#821) Signed-off-by: Todd Short <[email protected]> Improve caching and fix constant reconciles (#825) Improve performance by caching objects that only have ClusterExtension as owners. Signed-off-by: Varsha Prasad Narsing <[email protected]> Fix linter (#826) Signed-off-by: Varsha Prasad Narsing <[email protected]> Move helm-operator-plugin (#828) Signed-off-by: Todd Short <[email protected]> Really fix linter (#833) Signed-off-by: Todd Short <[email protected]>

tmshort requested a review from a team as a code owner April 16, 2024 15:55

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024

Fix suite_test.go

a4a403a

Signed-off-by: Todd Short <[email protected]>

tmshort force-pushed the helm-poc branch from 7381f9c to 304448b Compare April 16, 2024 16:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2024

varshaprasad96 reviewed Apr 16, 2024

View reviewed changes

tmshort force-pushed the helm-poc branch from 304448b to ad13b99 Compare April 16, 2024 18:09

tmshort force-pushed the helm-poc branch 2 times, most recently from 408661e to a7899fa Compare April 16, 2024 18:22

varshaprasad96 reviewed Apr 16, 2024

View reviewed changes

Copy over some rukpak code and replace BundleDeployment

110950f

Replace BundleDeployment in the Unpack APIs with a combination of BundleSource and ClusterExtension. It builds... Signed-off-by: Todd Short <[email protected]>

tmshort force-pushed the helm-poc branch from a7899fa to 110950f Compare April 16, 2024 18:57

varshaprasad96 approved these changes Apr 16, 2024

View reviewed changes

openshift-ci bot assigned varshaprasad96 Apr 16, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2024

fixup! Copy over some rukpak code and replace BundleDeployment

2e7f370

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2024

tmshort merged commit 631c758 into operator-framework:helm-poc Apr 16, 2024

tmshort deleted the helm-poc branch April 16, 2024 19:02

tmshort mentioned this pull request May 3, 2024

Helm PoC latest rebase - DO NOT MERGE #834

Closed

4 tasks

	SourceTypeGit SourceType = "git"
	SourceTypeConfigMaps SourceType = "configMaps"
	SourceTypeHTTP SourceType = "http"

✨ Starting Helm POC - pull in some rukpak #756

✨ Starting Helm POC - pull in some rukpak #756

Uh oh!

Conversation

tmshort commented Apr 16, 2024

Description

Reviewer Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Apr 16, 2024

Uh oh!

tmshort commented Apr 16, 2024

Uh oh!

varshaprasad96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort commented Apr 16, 2024

Uh oh!

varshaprasad96 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Apr 16, 2024

Uh oh!

Uh oh!