Add index build commands #106

kevinrizza · 2019-10-28T16:53:23Z

Added opm index add which generates indexes (either as a
dockerfile or shelling out to container tools) with additional bundles
Added opm index rm which generates an index based on a previous
index minus a specified operator

In order to write tests for these, added mock files for the new
containertools package via gomocks. Added a readme in the
doc/contributors/ folder that defines the process of updating these
mocks.

Note: this needs to be rebased on top of #89 once that merges. For now, marking this pr as WIP

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

ecordell

It would be nice to have at least one sanity e2e test that builds an image - can we check if podman is available in CI? Jeff recently added some ginkgo e2e tests for this repo.

ecordell · 2019-10-29T23:05:29Z

go.mod

@@ -3,6 +3,7 @@ module github.com/operator-framework/operator-registry
 require (
 	github.com/antihax/optional v0.0.0-20180407024304-ca021399b1a6
 	github.com/ghodss/yaml v1.0.0
+	github.com/go-bindata/go-bindata v3.1.2+incompatible // indirect


I don't think this should be here?

ecordell · 2019-10-29T23:08:10Z

pkg/containertools/labelreader.go

+
+// GetLabelsFromContainer takes a container image path as input, pulls that image
+// to the local environment and then inspects it for labels  
+func (r LabelReaderImpl) GetLabelsFromContainer(image string) (map[string]string, error) {


nit: Container generally refers to a running container, not an image. GetLabelsFromImage?

ecordell · 2019-10-29T23:08:48Z

pkg/containertools/dockerfilegenerator_test.go

+	require.Equal(t, dockerfile, expectedDockerfileWithPreviousIndex)
+}
+
+func TestGenerateDockerfileWithPreviousIndexDeleteOperators(t *testing.T) {


These should probably be table tests that at least test 0,1,n cases; ideally error cases as well

ecordell · 2019-10-29T23:09:42Z

docs/contributors/mocking.md

@@ -0,0 +1,15 @@
+# Mocking code in operator-registry


We migrated away from gomock to https://github.com/maxbrunsfeld/counterfeiter in OLM; it's been significantly lower maintenance than gomock.

This doesn't need to block, but if you check it out I think you'll find it easier 😄

ah, I was going off this:

https://github.com/operator-framework/operator-registry/tree/master/pkg/apprclient/mock

but now I realize that that's probably there because we never cleaned it up? I can look into this and see if I can do it quickly. We probably want to have all of the mocks in the project on the same framework at some point

ecordell · 2019-10-29T23:11:10Z

pkg/lib/index/indexer.go

@@ -0,0 +1,129 @@
+package index


why lib/index?

IMO the right way to do all of these is to stop putting functional logic into the cmd package and instead define that as just the steps to collect parameters from the terminal. Then have the first order function that actually implements those in the lib package. If we do this, it allows any arbitrary project to import our calls without actually having to import the Cobra stuff. Also, it makes testing the higher order library functions significantly easier.This is a first pass attempt to do that.

ecordell · 2019-10-30T11:05:30Z

pkg/containertools/dockerfilegenerator.go

+
+// GenerateIndexDockerfile builds a string representation of a dockerfile to use when building
+// an operator-registry index image
+func (g *DockerfileGeneratorImpl) GenerateIndexDockerfile(binarySourceImage, previousIndexImage string, bundlesToAdd, operatorsToDelete []string) (string, error) {


super-nit: my brain keeps reading "binary source image" as "source code written in binary" and not "the source image containing the binaries"

Yeah I had a lot of trouble coming up with these names (there are actually 3 different images involved). I was hoping someone could tell me this is bad and have a better idea. Any suggestions?

ecordell · 2019-10-30T11:07:10Z

pkg/containertools/dockerfilegenerator.go

+	// Generate the registry database
+	if len(bundlesToAdd) > 0 {
+		add := strings.Join(bundlesToAdd, ",")
+		dockerfile += fmt.Sprintf("RUN /build/bin/opm registry add \"%s\" -o \"index.db\"\n", add)


nit: seems weird to intentionally place our binaries in /build?

Yeah this is an artifact of the fact that the builder image (https://github.com/operator-framework/operator-registry/blob/master/upstream-builder.Dockerfile) just runs make on the source. If we wanted to change that we may have to create a new dockerfile for that build since I assume other things depend on that one.

Ah, I see that in the final image it's in a better location. Thanks!

ecordell · 2019-10-30T11:11:06Z

pkg/lib/index/indexer.go

+
+// Indexer allows the creation of index container images from scratch or
+// based on previous index images
+type Indexer interface {


This feels like it should be two interfaces, one for adding and one for removing?

ecordell · 2019-10-30T11:13:35Z

pkg/lib/index/indexer.go

+}
+
+// IndexerImpl is a struct implementation of the Indexer interface
+type IndexerImpl struct {


This is mostly a style thing, but I don't love <interface> interface and <interface>Impl struct, if you can think if something more descriptive.

For this one, I could imagine a a non-image-based implementation of this that operates on database files only. Then I would have IndexerImpl and DatabaseFileIndexerImpl, at which point why not have ImageIndexer and DatabaseFileIndexer so I can tell their intent apart?

ecordell · 2019-10-30T11:20:50Z

cmd/opm/index/add.go

+
+	indexer := index.NewIndexer(containerTool, logger)
+
+	request := index.AddToIndexRequest{


This is fine for now, for some other commands I started doing something similar to what oc does for building cobra commands:

https://github.com/operator-framework/operator-registry/blob/master/pkg/mirror/options.go#L7

It's not a huge problem the way it's written now, but it makes it difficult to use the defaulting/validation that's here if I wanted to use an indexer as a library (because that is done solely though the cobra flag setting).

interesting, i'll take a look

kevinrizza · 2019-10-30T18:14:45Z

@ecordell Updated based on your feedback, ptal

gallettilance

some minor nits + the need to rebase but looks great!

pkg/containertools/dockerfilegenerator_test.go

gallettilance · 2019-11-01T23:05:52Z

pkg/lib/indexer/indexer.go

+
+// AddToIndex is an aggregate API used to generate a registry index image with additional bundles
+func (i ImageIndexer) AddToIndex(request AddToIndexRequest) error {
+	databaseFile := defaultDatabaseFile


nit: spacing looks off in this function

gallettilance · 2019-11-01T23:07:19Z

pkg/lib/indexer/indexer.go

+
+// DeleteFromIndex is an aggregate API used to generate a registry index image 
+// without specific operators
+func (i ImageIndexer) DeleteFromIndex(request DeleteFromIndexRequest) error {


nit: spacing looks off in this function too

Motivation: In order to generate operator registry images, we need to create a high level command that allows CI and users to update the database based on previous images - Added `opm index add` which generates indexes (either as a dockerfile or shelling out to container tools) with additional bundles - Added `opm index delete` which generates an index based on a previous index minus a specified operator - Introduce counterfeiter as an interface mocker

gallettilance

Looks great!

ecordell · 2019-11-02T00:15:36Z

pkg/containertools/dockerfilegenerator_test.go

+}
+
+func TestGenerateDockerfile_EmptyBaseImage(t *testing.T) {
+	controller := gomock.NewController(t)


nit/nonblocking: I don't think this is used anymore?

ecordell · 2019-11-02T00:16:40Z

pkg/containertools/labelreader.go

+		if err != nil {
+			return nil, err
+		}
+		return data[0].Config.Labels, nil


nit/followup: should check length of data to be safe, right?

ecordell · 2019-11-02T00:16:57Z

pkg/containertools/labelreader_test.go

+package containertools_test
+
+import (
+    "fmt"


ecordell

/lgtm

openshift-ci-robot · 2019-11-02T00:20:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, gallettilance, kevinrizza

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ecordell]

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 label Oct 28, 2019

openshift-ci-robot requested review from ecordell and njhale October 28, 2019 16:53

openshift-ci-robot added the size/XXL label Oct 28, 2019

ecordell reviewed Oct 30, 2019

View reviewed changes

kevinrizza force-pushed the index-command branch from 0e8cacd to 24a083c Compare October 30, 2019 18:14

openshift-ci-robot added the needs-rebase label Oct 30, 2019

kevinrizza force-pushed the index-command branch from 24a083c to 52c3383 Compare November 1, 2019 21:02

openshift-ci-robot removed the needs-rebase label Nov 1, 2019

gallettilance reviewed Nov 1, 2019

View reviewed changes

kevinrizza force-pushed the index-command branch from 9fa3022 to 1be113b Compare November 2, 2019 00:01

kevinrizza changed the title ~~WIP: Add index build commands~~ Add index build commands Nov 2, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress label Nov 2, 2019

gallettilance approved these changes Nov 2, 2019

View reviewed changes

ecordell reviewed Nov 2, 2019

View reviewed changes

pkg/containertools/labelreader_test.go

package containertools_test

import (

"fmt"

Copy link

Member

ecordell Nov 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: gofmt

ecordell approved these changes Nov 2, 2019

View reviewed changes

openshift-ci-robot assigned ecordell Nov 2, 2019

openshift-ci-robot added the lgtm label Nov 2, 2019

openshift-ci-robot added the approved label Nov 2, 2019

openshift-merge-robot merged commit f30cfde into operator-framework:master Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add index build commands #106

Add index build commands #106

kevinrizza commented Oct 28, 2019 •

edited

Loading

ecordell left a comment

ecordell Oct 29, 2019

ecordell Oct 29, 2019

ecordell Oct 29, 2019

ecordell Oct 29, 2019

kevinrizza Oct 30, 2019

ecordell Oct 29, 2019

kevinrizza Oct 30, 2019

ecordell Oct 30, 2019

kevinrizza Oct 30, 2019

ecordell Oct 30, 2019

kevinrizza Oct 30, 2019

ecordell Oct 31, 2019

ecordell Oct 30, 2019

kevinrizza Oct 30, 2019

ecordell Oct 30, 2019

ecordell Oct 30, 2019

kevinrizza Oct 30, 2019

kevinrizza commented Oct 30, 2019

gallettilance left a comment

gallettilance Nov 1, 2019

gallettilance Nov 1, 2019

gallettilance left a comment

ecordell Nov 2, 2019 •

edited

Loading

ecordell Nov 2, 2019

ecordell Nov 2, 2019

ecordell left a comment

openshift-ci-robot commented Nov 2, 2019


		indexer := index.NewIndexer(containerTool, logger)

		request := index.AddToIndexRequest{

Add index build commands #106

Add index build commands #106

Conversation

kevinrizza commented Oct 28, 2019 • edited Loading

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrizza commented Oct 30, 2019

gallettilance left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gallettilance left a comment

Choose a reason for hiding this comment

ecordell Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Nov 2, 2019

kevinrizza commented Oct 28, 2019 •

edited

Loading

ecordell Nov 2, 2019 •

edited

Loading