Expose bundle data from bundle image by jpeeler · Pull Request #94 · operator-framework/operator-registry

jpeeler · 2019-10-01T20:03:15Z

This provides a command to expose data in a bundle image and parses both /manifests and /metadata as proposed in operator-framework/operator-lifecycle-manager#1054.

~~This might need some shuffling around as I'm not sure the sqlite package is the best place for this code to live. Also, currently it includes another WIP PR which is likely to change.~~

This includes functionality for launching the container to execute the new serve command, to be called from OLM once this and other PRs are pulled in.

OLM-1275

jpeeler · 2019-10-03T20:34:11Z

This update includes the code for launching a container to execute the new serve command and is feature complete, minus the tests.

jpeeler · 2019-10-09T21:13:37Z

I just need to plumb the e2e test (potentially) into the makefile and CI. The WIP commit (related to CLI handling) referenced above has been removed.

awgreene

Great work! I had a few nits and general question but the logic looks great!

awgreene · 2019-10-10T22:11:31Z

+)
+
+// configmap keys can contain underscores, but configmap names can not
+var unallowedKeyChars = regexp.MustCompile("[^-A-Za-z0-9_.]")


Rather than whitelisting valid characters wouldn't it be better to check for valid k8s object names?

For example, a-- would get through this. I understand that this is being used to fix invalid configmap names, but I'm not sure if that's something the tool should be responsible for.

There was some discussion as to whether or not the code here should be responsible for validating "valid" file names ("valid" because technically it's a configmap implementation detail leaking into what's considered valid from a user perspective). However, given how short this code is, I personally like ensuring that things don't explode. And since the resource names aren't in play here, I believe the chosen regex is best.

awgreene · 2019-10-10T22:21:21Z

+}
+
+func TranslateInvalidChars(input string) string {
+	validConfigMapKey := unallowedKeyChars.ReplaceAllString(input, "~")


It is not apparent to me why you are providing ~ as an input (even after googling). Could you explain?

This ties into the above comment. If you want to argue that failing instead of translating to valid is best, then this code is up for potential removal. But I randomly chose the tilda character as a character that is typically rarely used to replace any invalid characters. In practice, I hope the string is not modified, but did make sure to include a warning message to point out the potential issue.

jpeeler · 2019-10-14T18:06:48Z

/retest

This verifies that the configmap is being properly read from the extracted data of a bundle image (from operator-framework#94).

ecordell · 2019-10-18T12:36:39Z

@@ -0,0 +1,3 @@
+annotations:
+  operators.coreos.com.bundle.resources: "manifests+metadata"


This is the only change I think we need. Please see my comment on Vu's PR: #97 (comment)

This should be a minor change, otherwise everything looks great!

Before I lgtm, do you want to hold this for further testing on the OLM side? Or would you rather merge and make new PRs if something comes up?

I think it's easier to go ahead and get in. Hopefully any follow ups are pretty minor.

Previously, the package manifest data was going to be included as a file within the bundle. The planning changed and this updates the implementation to match. (This commit can be squashed, but was left as separate for review purposes.) The configmap annotations for manifest data has been made as a requirement. Also, this PR now depends on operator-framework#94.

This verifies that the configmap is being properly read from the extracted data of a bundle image (from operator-framework#94).

Allows code reuse throughout operator-registry.

This implements "Populate" for writing bundle data to a configmap. Both subdirectories manifests and metadata are parsed for files to be stored in the configmap. Also implements a function for deploying the bundle image via a job.

This command serves the manifest data from an image bundle into a configmap via `opm alpha bundle extract`.

Essentially what is produced by `ginkgo bootstrap`, but without using dot-imports. The package name is intentionally chosen to be outside of the code under test in order to encourage not reaching into the internals.

The image-bundle directory contains files for the data inside a bundle image in addition to buildling both the bundle and init container images.

ecordell

/lgtm

I had some nits, but they can be resolved in followups.

ecordell · 2019-10-29T19:20:38Z

+	clientset     *kubernetes.Clientset
+}
+
+func NewConfigMapLoaderForDirectory(configMapName, namespace, manifestsDir, kubeconfig string) *ConfigMapWriter {


nit: NewConfigMapWriterForDirectory

nit: might want to use the functional option pattern for arguments we've been using elsewhere

ecordell · 2019-10-29T19:23:41Z

+// the responsibility of the caller to delete the job, the pod, and the configmap
+// when done. This function is intended to be called from OLM, but is put here
+// for locality.
+func LaunchBundleImage(kubeclient kubernetes.Interface, bundleImage, initImage, namespace string) (*corev1.ConfigMap, *batchv1.Job, error) {


nit: should be able to set the path to find opm in initImage?

ecordell · 2019-10-29T19:25:40Z

@@ -0,0 +1,773 @@
+package e2e_test


openshift-ci-robot · 2019-10-29T19:26:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, jpeeler

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell,jpeeler]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 1, 2019

openshift-ci-robot requested review from alecmerdler and ecordell October 1, 2019 20:03

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 1, 2019

jpeeler force-pushed the expose-bundle-data branch from a133142 to 33248b4 Compare October 3, 2019 20:33

jpeeler force-pushed the expose-bundle-data branch from 33248b4 to d18f84f Compare October 9, 2019 21:10

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2019

jpeeler force-pushed the expose-bundle-data branch from d18f84f to ec6e3c0 Compare October 9, 2019 21:11

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2019

jpeeler changed the title ~~WIP: Expose bundle data from bundle image~~ Expose bundle data from bundle image Oct 9, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 9, 2019

awgreene requested changes Oct 10, 2019

View reviewed changes

jpeeler force-pushed the expose-bundle-data branch from ec6e3c0 to e59b816 Compare October 11, 2019 18:46

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2019

jpeeler force-pushed the expose-bundle-data branch from e59b816 to b40c9b8 Compare October 11, 2019 18:49

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2019

ecordell reviewed Oct 12, 2019

View reviewed changes

Comment thread pkg/sqlite/configmap_writer.go Outdated

ecordell reviewed Oct 12, 2019

View reviewed changes

Comment thread cmd/bundle-extractor/main.go Outdated

ecordell reviewed Oct 12, 2019

View reviewed changes

Comment thread pkg/sqlite/configmap_writer.go Outdated

ecordell reviewed Oct 12, 2019

View reviewed changes

Comment thread pkg/sqlite/configmap_writer.go

jpeeler force-pushed the expose-bundle-data branch from b40c9b8 to 220ec6a Compare October 14, 2019 14:22

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019

jpeeler force-pushed the expose-bundle-data branch from 220ec6a to a98ec3c Compare October 14, 2019 14:23

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019

jpeeler force-pushed the expose-bundle-data branch from a98ec3c to ef47feb Compare October 14, 2019 16:09

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019

jpeeler force-pushed the expose-bundle-data branch from ef47feb to c94d43f Compare October 14, 2019 16:09

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2019

jpeeler pushed a commit to tkashem/operator-registry that referenced this pull request Oct 14, 2019

test(unit): add test case from generated configmap

eae7889

This verifies that the configmap is being properly read from the extracted data of a bundle image (from operator-framework#94).

jpeeler mentioned this pull request Oct 14, 2019

(feat) Load operator registry bundle from ConfigMap #92

Merged

ecordell reviewed Oct 18, 2019

View reviewed changes

jpeeler force-pushed the expose-bundle-data branch from c94d43f to f6fb0e4 Compare October 18, 2019 16:46

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2019

jpeeler force-pushed the expose-bundle-data branch from f6fb0e4 to 05cd9a9 Compare October 18, 2019 16:47

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2019

jpeeler pushed a commit to tkashem/operator-registry that referenced this pull request Oct 28, 2019

test(unit): add test case from generated configmap

7804c8c

This verifies that the configmap is being properly read from the extracted data of a bundle image (from operator-framework#94).

jpeeler force-pushed the expose-bundle-data branch from 05cd9a9 to 371a5c2 Compare October 28, 2019 21:51

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2019

Jeff Peeler added 5 commits October 28, 2019 17:51

chore(client): move k8s clientset generation

84f9803

Allows code reuse throughout operator-registry.

feat(configmap): add configmap writer

009bdf7

This implements "Populate" for writing bundle data to a configmap. Both subdirectories manifests and metadata are parsed for files to be stored in the configmap. Also implements a function for deploying the bundle image via a job.

feat(cli): add bundle extractor binary

5ffa448

This command serves the manifest data from an image bundle into a configmap via `opm alpha bundle extract`.

test(e2e): add initial test bootstrap

2186398

Essentially what is produced by `ginkgo bootstrap`, but without using dot-imports. The package name is intentionally chosen to be outside of the code under test in order to encourage not reaching into the internals.

test(e2e): add bundle data image extraction test

2c7ba05

The image-bundle directory contains files for the data inside a bundle image in addition to buildling both the bundle and init container images.

jpeeler force-pushed the expose-bundle-data branch from 371a5c2 to 2c7ba05 Compare October 28, 2019 21:52

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2019

ecordell approved these changes Oct 29, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Oct 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2019

openshift-merge-robot merged commit 8df762a into operator-framework:master Oct 29, 2019

		@@ -0,0 +1,3 @@
		annotations:
		operators.coreos.com.bundle.resources: "manifests+metadata"

Conversation

jpeeler commented Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpeeler commented Oct 3, 2019

Uh oh!

jpeeler commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpeeler commented Oct 14, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

ecordell Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

openshift-ci-robot commented Oct 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jpeeler commented Oct 1, 2019 •

edited

Loading

jpeeler commented Oct 9, 2019 •

edited

Loading

ecordell Oct 29, 2019 •

edited

Loading