Opm test #196

gallettilance · 2020-02-25T15:22:11Z

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 · 2020-02-26T15:14:36Z

opm-test.sh

 echo "pushing index image"
 $containerTool push $indexImage
+
 echo "sleep for 30s before pulling image"


why do we need to sleep?

Oh good catch. I had this for when I was pulling from the same tag locally on every run

If there is a reason for the 30s wait, please add a comment.

ecordell · 2020-02-26T15:15:26Z

opm-test.sh

@@ -1,26 +1,58 @@
 #!/bin/bash


could we make this a go file that execs opm (or just exercises the commands directly)?

that will also let you use build tags to selectively build for os

awgreene

Looks great @gallettilance! A few nits but nothing blocking.

awgreene · 2020-02-26T15:17:40Z

opm-test.sh

-read -p "docker or podman: " containerTool
+username=$2
+token=$3
+echo "logging in to quay"


nit: into

awgreene · 2020-02-26T15:18:17Z

opm-test.sh

+docker login -u "$username" -p "$token" quay.io
+
+echo "Generating Random Tag"
+bundleTag1=$(cat /dev/urandom | tr -dc 'a-zA-Z0-9' | fold -w 6 | head -n 1)


Could this be a function?

opm-test.sh

awgreene · 2020-02-26T15:21:41Z

opm-test.sh


 echo "building opm"
 go build -mod=vendor  -o bin/opm ./cmd/opm
+echo "building initializer"
+go build -mod=vendor  -o bin/initializer ./cmd/initializer


Nit: Double space between mod and o.

awgreene · 2020-02-26T15:22:03Z

opm-test.sh

+indexImage="quay.io/olmtest/e2e-index:$indexTag"
+
+if [ $# -eq 0 ]; then
+    echo "No container tool argument supplied"


Nit: Technically this means no arguments were supplied right?

awgreene · 2020-02-26T15:22:56Z

opm-test.sh

 echo "pushing index image"
 $containerTool push $indexImage
+
 echo "sleep for 30s before pulling image"


If there is a reason for the 30s wait, please add a comment.

awgreene · 2020-02-26T15:23:35Z

opm-test.sh

+fi
+
+containerTool="$1"
+echo "Running with container tool $containerTool"


Most of the echos are lowercase, please make this consistent.

.travis.yml

ecordell · 2020-02-28T14:13:31Z

opm-test.go

+	return loader.Populate()
+}
+
+func main() {


Why not use the testing package and run through all of the options (that exist for the target os?)

kevinrizza

Mostly nits, but I am interested in why we aren't using go test for this

kevinrizza · 2020-02-28T14:13:07Z

.travis.yml

- if [[ "$TRAVIS_OS_NAME" == "windows" ]]; then mingw32-make build; fi
- if [[ "$TRAVIS_OS_NAME" != "windows" ]]; then make unit build; fi
+script: |
+  if [[ "$TRAVIS_OS_NAME" == "windows" ]]; then


Since this seems like technical debt to actually enable these tests in multiple environments, can we just run the tests in only linux to start and create issues to get them enabled everywhere else at a later time? Even if we don't remove these steps for now, I think it would be valuable to track it somewhere.

.travis.yml

opm-test.go

.travis.yml

benluddy · 2020-03-12T03:46:50Z

/hold

benluddy · 2020-03-12T18:08:08Z

@kevinrizza @ecordell @awgreene

I updated this change to run the new tests as ginkgo specs. They'll be skipped when running against PRs from forks, which won't have access to registry credentials (https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions). PTAL!

https://github.com/operator-framework/operator-registry/pull/196/checks

benluddy · 2020-03-12T18:09:42Z

/hold cancel

benluddy · 2020-03-13T00:22:06Z

/test e2e-aws

benluddy · 2020-03-13T01:30:58Z

/test e2e-aws

benluddy · 2020-03-13T13:40:36Z

/test e2e-aws

benluddy · 2020-03-13T19:18:23Z

/test e2e-aws

Makefile

ecordell · 2020-03-13T21:23:39Z

pkg/lib/registry/registry.go

@@ -5,9 +5,9 @@ import (
 	"database/sql"
 	"fmt"

-	"github.com/sirupsen/logrus"
-
 	"github.com/operator-framework/operator-registry/pkg/sqlite"


nit: we have been using default - library - current module order for imports

pkg/lib/registry/registry.go

ecordell

/lgtm

openshift-ci-robot · 2020-03-13T22:16:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, 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 added the size/M label Feb 25, 2020

openshift-ci-robot requested review from dinhxuanvu and ecordell February 25, 2020 15:23

gallettilance force-pushed the opm-test branch 4 times, most recently from ecf008a to 5eb0e01 Compare February 26, 2020 15:01

ecordell reviewed Feb 26, 2020

View reviewed changes

awgreene approved these changes Feb 26, 2020

View reviewed changes

gallettilance force-pushed the opm-test branch 4 times, most recently from e64e717 to 6d71aee Compare February 27, 2020 20:39

openshift-ci-robot added size/L and removed size/M labels Feb 27, 2020

ecordell reviewed Feb 28, 2020

View reviewed changes

kevinrizza requested changes Feb 28, 2020

View reviewed changes

benluddy reviewed Feb 28, 2020

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

openshift-ci-robot added needs-rebase size/XXL and removed size/L labels Mar 12, 2020

benluddy force-pushed the opm-test branch 5 times, most recently from 35987a9 to edde384 Compare March 12, 2020 03:42

openshift-ci-robot added the do-not-merge/hold label Mar 12, 2020

benluddy force-pushed the opm-test branch from edde384 to eeb0a1c Compare March 12, 2020 04:15

benluddy force-pushed the opm-test branch from eeb0a1c to 2440e13 Compare March 12, 2020 04:36

openshift-ci-robot removed the needs-rebase label Mar 12, 2020

benluddy force-pushed the opm-test branch 6 times, most recently from ddf1586 to 727f39a Compare March 12, 2020 17:39

benluddy requested review from ecordell, kevinrizza and awgreene March 12, 2020 18:08

openshift-ci-robot removed the do-not-merge/hold label Mar 12, 2020

benluddy mentioned this pull request Mar 13, 2020

Fix typo in opm-test.sh #215

Closed

ecordell reviewed Mar 13, 2020

View reviewed changes

Add opm E2E test.

Verified

This commit was signed with the committer’s verified signature.

benluddy Ben Luddy

GPG key ID: 893826B41C2B9C2A

Verified
Learn about vigilant mode

626af90

benluddy force-pushed the opm-test branch from 727f39a to 626af90 Compare March 13, 2020 22:13

ecordell approved these changes Mar 13, 2020

View reviewed changes

openshift-ci-robot assigned ecordell Mar 13, 2020

openshift-ci-robot added the lgtm label Mar 13, 2020

openshift-ci-robot added the approved label Mar 13, 2020

openshift-merge-robot merged commit 100f3bc into master Mar 13, 2020

benluddy deleted the opm-test branch March 14, 2020 00:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Opm test #196

Opm test #196

gallettilance commented Feb 25, 2020

ecordell Feb 26, 2020

gallettilance Feb 26, 2020

awgreene Feb 26, 2020 •

edited

Loading

ecordell Feb 26, 2020 •

edited

Loading

ecordell Feb 26, 2020

gallettilance Feb 26, 2020

awgreene left a comment

awgreene Feb 26, 2020

awgreene Feb 26, 2020

awgreene Feb 26, 2020

awgreene Feb 26, 2020

awgreene Feb 26, 2020 •

edited

Loading

awgreene Feb 26, 2020

ecordell Feb 28, 2020

kevinrizza left a comment

kevinrizza Feb 28, 2020

gallettilance Feb 28, 2020

benluddy commented Mar 12, 2020

benluddy commented Mar 12, 2020

benluddy commented Mar 12, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

ecordell Mar 13, 2020

ecordell left a comment

openshift-ci-robot commented Mar 13, 2020

Opm test #196

Opm test #196

Conversation

gallettilance commented Feb 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

ecordell Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene 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

awgreene Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

benluddy commented Mar 12, 2020

benluddy commented Mar 12, 2020

benluddy commented Mar 12, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

benluddy commented Mar 13, 2020

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 13, 2020

awgreene Feb 26, 2020 •

edited

Loading

ecordell Feb 26, 2020 •

edited

Loading

awgreene Feb 26, 2020 •

edited

Loading