Add index build commands by kevinrizza · Pull Request #106 · operator-framework/operator-registry

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

 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

+
+// 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

+	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

@@ -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

@@ -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

+
+// 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

+	// 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

+
+// 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

+}
+
+// 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

+
+	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!

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

+
+// 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

+
+// 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

+}
+
+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

+		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

+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

Details

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 Indicates that a PR should not merge because it is a 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 Denotes a PR that changes 1000+ lines, ignoring generated files. 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 Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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 Indicates that a PR should not merge because it is a 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

Comment thread pkg/containertools/labelreader_test.go

package containertools_test

import (

"fmt"

Copy link
Copy Markdown

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 Indicates that a PR is ready to be merged. label Nov 2, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2019

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


		indexer := index.NewIndexer(containerTool, logger)

		request := index.AddToIndexRequest{

Conversation

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

Uh oh!

Uh oh!

ecordell 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!

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!

kevinrizza commented Oct 30, 2019

Uh oh!

gallettilance 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!

gallettilance left a comment

Choose a reason for hiding this comment

Uh oh!

ecordell Nov 2, 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!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Nov 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kevinrizza commented Oct 28, 2019 •

edited

Loading

ecordell Nov 2, 2019 •

edited

Loading