Adding opm CLI #96

gallettilance · 2019-10-10T20:31:43Z

Description of the change:

Adding a new cli to operator-registry called opm. It can add bundles to a database, delete packages from that db and serve its contents at a specified host and port.

Having built the binaries, a user can run the old binaries

./bin/initializer ...
./bin/appregistry-server ...
./bin/registry-server ...
./bin/configmap-server ...

or can use the new cli that supports (and will support) our new operator bundle format. Here is an example flow:

Build the database (or add to an existing one) using

./bin/opm registry add -d=bundles.db -m=quay.io/username/imagename

Remove operators from an existing db using

./bin/opm registry rm -d=bundles.db -o="operator-package-name"

Serve the db using (can optionally specify a host and port)

./bin/opm registry serve -t=path/to/file.log -d=bundles.db

Implemented here

In this PR we are adding the opm cli and a sub-command registry. The registry command itself has a subcommand called serve which serves the contents of an operator-registry database (created from an index image, using bundles in the new format, etc - note that how to create and modify that database will be in a follow up PR)

Motivation for the change:

This is a first step toward creating new commands to interface with operator-registry as part of its corresponding epic.

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

njhale

Wow, looking really great! I jotted down some thoughts I had:

cmd/appregistry-server/main.go

cmd/cli/opmregistry.go

njhale · 2019-10-11T13:49:20Z

cmd/cli/registryserver.go

+			return nil
+		},
+
+		RunE: runRegistryServerCmdFunc,


Another option is to call runOpmRegistryServeCmdFunc instead and drop the definition of runRegistryServerCmdFunc.

cmd/cli/opmregistry.go

njhale · 2019-10-11T13:57:46Z

cmd/cli/opmregistry.go

+)
+
+// NewOpmRegistryCmd returns the appregistry-server command
+func NewOpmRegistryCmd() *cobra.Command {


What about NewRegistryCmd instead?

njhale · 2019-10-11T14:10:59Z

Makefile

 MOD_FLAGS := $(shell (go version | grep -q -E "1\.(11|12)") && echo -mod=vendor)
-CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | xargs -I{} basename {}))
+CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | grep -Fv "github.com/operator-framework/operator-registry/cmd/cli" |xargs -I{} basename {}))


If we keep the command declarations in separate top level packages, we don't need to do this right?

You mean put this in pkg instead of cmd?

I mean cmd/initializer, cmd/registry, cmd/opm, and forgo extracting the commands out to cmd/cli -- but it seems like everyone has a different opinion on this, so feel free to ignore.

njhale · 2019-10-11T14:11:56Z

cmd/opm/main.go

+
+func main() {
+	rootCmd := &cobra.Command{
+		Use:   "opm",


pkg/sqlite/load.go

ecordell · 2019-10-12T17:02:36Z

@gallettilance I made a separate PR (#98) to address the foreign key issues; can you remove it from this one so that tests are passing?

kevinrizza

Had a few nits, otherwise this is looking good.

kevinrizza · 2019-10-15T13:32:58Z

Makefile

@@ -1,5 +1,5 @@
 MOD_FLAGS := $(shell (go version | grep -q -E "1\.(11|12)") && echo -mod=vendor)
-CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | xargs -I{} basename {}))
+CMDS  := $(addprefix bin/, $(shell go list $(MOD_FLAGS) ./cmd/... | grep -v opm/... | xargs -I{} basename {}))


Do we want to continue to use go list like this? Maybe we should just explicitly list the things we build rather than rely on using this command and ignoring part of the result? Or at least should we have a heuristic for determining it (like only look at the packages in the root of ./cmd/?

@dinhxuanvu Any thoughts on this? I know you most likely have a similar problem with the bundle build command you are adding.

Good point. Curious to see what @dinhxuanvu 's opinion is

So the plan for me is to separate the functional code of my bundle PR to have it as a package living under /pkg and the cobra command code (the rest) will live under /opm/bundle/... and it will consume the bundle package.

Why do we want to omit opm from the build?

@dinhxuanvu I thought we wanted to expose the bundle build as a command that wasn't built as part of any of our existing utilities? and then we create another pr against operator-sdk that consumed that cobra thing? We can create our own test utility that can also use that cobra thing but we won't publish it, just use it to test. but I was expecting it to NOT be part of opm.

@ecordell I think he just meant that he doesn't want the subcommand packages to be part of the list of things that should be called by go build. Those packages don't have main.go files so the command that's in the makefile now will throw an error.

I think this will do what we want:

CMDS := $(addprefix bin/, $(shell ls ./cmd))

@kevinrizza You are correct. The ultimate usage for bundle library would be on SDK side. The users won't use bundle commands as a part of opm binary. I'm planning to add bundle commands with hidden flag on cobra. So we know they are there and available for internal use but they won't show up in the list of available commands.

kevinrizza · 2019-10-15T13:33:17Z

cmd/opm/main.go

+	rootCmd := &cobra.Command{
+		Use:   "opm",
+		Short: "operator package manager",
+		Long:  "Top level CLI for operator-registry",


Is this long description accurate anymore?

kevinrizza · 2019-10-15T13:34:01Z

cmd/opm/registry/cmd.go

+func NewOpmRegistryCmd() *cobra.Command {
+	rootCmd := &cobra.Command{
+		Use:   "registry",
+		Short: "registry",


I think you want Short to have a slightly longer description that defines what it actually does. Otherwise --help won't really tell you anything.

kevinrizza · 2019-10-15T13:34:15Z

cmd/opm/registry/serve.go

+func newRegistryServeCmd() *cobra.Command {
+	rootCmd := &cobra.Command{
+		Use:   "serve",
+		Short: "serve",


Same comment as above about Short

ecordell · 2019-10-17T14:09:38Z

/lgtm

/retest

openshift-ci-robot · 2019-10-17T14:09:41Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 requested review from dinhxuanvu and ecordell October 10, 2019 20:31

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

njhale reviewed Oct 11, 2019

View reviewed changes

gallettilance force-pushed the opm-cli branch 2 times, most recently from c9357f3 to 49d80f5 Compare October 15, 2019 13:29

openshift-ci-robot added size/L and removed size/XXL labels Oct 15, 2019

kevinrizza requested changes Oct 15, 2019

View reviewed changes

gallettilance force-pushed the opm-cli branch 2 times, most recently from 1501048 to 8bf56ad Compare October 17, 2019 13:54

openshift-ci-robot added needs-rebase size/XXL and removed size/L labels Oct 17, 2019

Adding opm cli

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

958896d

gallettilance force-pushed the opm-cli branch from 8bf56ad to 958896d Compare October 17, 2019 13:58

openshift-ci-robot added size/L and removed needs-rebase size/XXL labels Oct 17, 2019

openshift-ci-robot assigned ecordell Oct 17, 2019

openshift-ci-robot added the lgtm label Oct 17, 2019

openshift-ci-robot added the approved label Oct 17, 2019

openshift-merge-robot merged commit b81f428 into operator-framework:master Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding opm CLI #96

Adding opm CLI #96

gallettilance commented Oct 10, 2019 •

edited

Loading

njhale left a comment

njhale Oct 11, 2019

njhale Oct 11, 2019

njhale Oct 11, 2019

gallettilance Oct 14, 2019

njhale Oct 14, 2019

njhale Oct 11, 2019

ecordell commented Oct 12, 2019 •

edited

Loading

kevinrizza left a comment

kevinrizza Oct 15, 2019

gallettilance Oct 15, 2019

dinhxuanvu Oct 15, 2019

ecordell Oct 15, 2019

kevinrizza Oct 15, 2019

ecordell Oct 15, 2019

dinhxuanvu Oct 16, 2019

kevinrizza Oct 15, 2019

gallettilance Oct 15, 2019

kevinrizza Oct 15, 2019

gallettilance Oct 15, 2019

kevinrizza Oct 15, 2019

gallettilance Oct 15, 2019

ecordell commented Oct 17, 2019

openshift-ci-robot commented Oct 17, 2019

Adding opm CLI #96

Adding opm CLI #96

Conversation

gallettilance commented Oct 10, 2019 • edited Loading

njhale 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

ecordell commented Oct 12, 2019 • edited Loading

kevinrizza 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

ecordell commented Oct 17, 2019

openshift-ci-robot commented Oct 17, 2019

gallettilance commented Oct 10, 2019 •

edited

Loading

ecordell commented Oct 12, 2019 •

edited

Loading