Create Makefile target to install operator & test #33

Avni-Sharma · 2019-03-21T12:17:59Z

Refer: https://jira.coreos.com/browse/ODC-189

This pull request introduces a new Makefile target to run the Operator Lifecycle Manager (OLM) integration test. The target is test-olm-integration, and you can run it like this:

make test-olm-integration

Before running the above, you should set environment variables listed here:

export QUAY_USERNAME=
export QUAY_PASSWORD=
export DEVCONSOLE_OPERATOR_IMAGE=quay.io/<username>/devconsole-operator
export DEVCONSOLE_OPERATOR_REGISTRY_IMAGE=quay.io/<username>/operator-registry

The QUAY_USERNAME and QUAY_PASSWORD is used to login to Quay. The DEVCONSOLE_OPERATOR_IMAGE environment variable is pointing to the operator image location without the tag. TheDEVCONSOLE_OPERATOR_REGISTRY_IMAGE is pointing to the registry image without the tag.

Refer: https://jira.coreos.com/browse/ODC-189

centos-ci · 2019-03-21T12:18:01Z

Can one of the admins verify this patch?

Avni-Sharma · 2019-03-21T12:25:37Z

This is a draft. There are a few hard coded variables which need to be fixed. The operator image should be available as a prerequisite for this PR.
I would like to get feedback on the approach taken here.

sbose78 · 2019-03-21T13:14:38Z

make/test.mk

@@ -127,6 +127,53 @@ e2e-cleanup:
 	oc login -u system:admin
 	oc delete project devconsole-e2e-test || true

+.PHONY: test-olm-integration


Could we move this to a bash script and call it from here?

Makefiles and complicated scripts are not best friends. But if you can split those make targets into multiple ones that depend on each other, it is superior to having a big bash file IMHO.

We will split it into smaller multiple targets.

kwk · 2019-03-21T13:55:40Z

make/test.mk

@@ -127,6 +127,53 @@ e2e-cleanup:
 	oc login -u system:admin
 	oc delete project devconsole-e2e-test || true

+.PHONY: test-olm-integration


Makefiles and complicated scripts are not best friends. But if you can split those make targets into multiple ones that depend on each other, it is superior to having a big bash file IMHO.

kwk · 2019-03-21T13:58:43Z

make/test.mk

+	cp  $(CUR_DIR)/manifests/devopsconsole/$(devopsconsole_version)/$(devopsconsole_csv).clusterserviceversion.yaml $(CUR_DIR)/devopsconsole_tmp.yaml
+
+	sed -e "s,REPLACE_IMAGE,quay.io/baijum/devopsconsole-operator:0.1.0," -i $(CUR_DIR)/manifests/devopsconsole/${devopsconsole_version}/${devopsconsole_csv}.clusterserviceversion.yaml
+	docker build -f $(CUR_DIR)/test/olm/Dockerfile $(CUR_DIR) -t $(DEVOPSCONSOLE_OPERATOR_REGISTRY):$(devopsconsole_version)


Can't we have multiple more simply steps that are testable on their own? For example, build the docker image, then you can do docker images to see if it is there. If this remains in a makefile I think we should have targets depend on each other.
If one of those commands in this target fails, you probably have a hard time finding out how to test only one bit in particular.

kwk · 2019-03-21T14:00:44Z

make/test.mk

+
+	operator-sdk test local ./test/olm/ --go-test-flags "-v -parallel=1"
+
+	#TODO: the below command should be replaced with the actual test command.


Please use #TODO(Avni-Sharma): ... to help others know that it is @Avni-Sharma who's supposed to fix this or who added this comment.

kwk · 2019-03-21T14:03:55Z

make/test.mk

+	#TODO: the below command should be replaced with the actual test command.
+	#go test ./test/olm/... -root=$(PWD) -kubeconfig=$(HOME)/.kube/config -globalMan deploy/test/global-manifests.yaml -namespacedMan deploy/test/namespace-manifests.yaml -v -parallel=1 -singleNamespace
+
+# .PHONY: olm-integration-setup


This is what I meant by have the make targets depend on each other.

sbose78 · 2019-03-22T09:23:22Z

/shrug

Following the operator-sdk testing framework. Referred Corrine's/Tina's PR.

…makefile

sbose78

Today I spent quite some time debugging the absence of a rolebinding which was present in deploy/.. but absent when deployed using the registry.

And then I realized that our csv yaml needed a fix.

Doesn't necessarily have to be a part of this PR, but a validation to check if the service account can see relevant resources in a different namespace is going to be very useful.

sbose78 · 2019-03-22T17:44:58Z

make/test.mk

+	oc login -u system:admin
+	oc create -f https://raw.githubusercontent.com/operator-framework/operator-lifecycle-manager/master/deploy/upstream/quickstart/olm.yaml
+	#Baiju's image. Replace it with the actual image
+	$(eval image := "quay.io/baijum/devopsconsole-operator:0.1.0")                                                                                                                                              


Eventually, you could tag it as quay.io/redhat-developers/devopsconsole-operator:test{commitid}

This will be coming through DEVOPSCONSOLE_OPERATOR_IMAGE environment variable

sbose78 · 2019-03-22T17:50:07Z

make/test.mk

+
+	#oc tag 172.30.1.1:5000/$(namespace)/operator-registry:latest $(DEVOPSCONSOLE_OPERATOR_REGISTRY):$(devopsconsole_version)
+	sed -e "s,REPLACE_IMAGE,$(DEVOPSCONSOLE_OPERATOR_REGISTRY):$(devopsconsole_version)," $(CUR_DIR)/test/olm/catalog_source.yaml | oc create -f - 
+	oc create -f $(CUR_DIR)/test/olm/subscription.yaml


The creation of the CatalogSource is missing?

The previous line creates CatalogSource.

sbose78 · 2019-03-22T17:52:42Z

make/test.mk

+	oc create -f $(CUR_DIR)/test/olm/subscription.yaml
+	sleep 1m
+	oc get catalogsources -n olm
+	oc get subscriptions -n operators


Validate the status of the subscription?

.. and valid that the 'deployment' is 'ready'.

All validation will be moved to test written in Go.

sbose78 · 2019-03-22T17:58:41Z

make/test.mk

+
+# .PHONY: olm-integration-setup
+# olm-integration-setup:  e2e-cleanup
+# 	oc new-project devconsole-e2e-test || true


Btw, it has been seen that deletion of the namespace at the end of the test takes time , which sometimes blocks the next test run because creation of the namespace with the same name is attempted.

You could consider creating a unique namespace name every time.

So ideally when should a cleanup happen?

You could create a namespace with a random string appended to it "devconsole-e2e-test-xyz" and delete it. Let the 'garbage collection' happen at its own pace by openshift.

baijum · 2019-03-27T03:22:14Z

make/test.mk

@@ -88,6 +88,9 @@ ALL_PKGS_EXCLUDE_PATTERN = 'vendor\|app\|tool\/cli\|design\|client\|test'
 GOANALYSIS_PKGS_EXCLUDE_PATTERN="vendor|app|client|tool/cli"
 GOANALYSIS_DIRS=$(shell go list -f {{.Dir}} ./... | grep -v -E $(GOANALYSIS_PKGS_EXCLUDE_PATTERN))

+RANDOM := $(shell uuidgen)


Once PR #34 is merged, remove this line.

Removed duplicate.

sbose78 · 2019-03-27T17:05:26Z

/assign @baijum

baijum

LGTM.

@Avni-Sharma Let's wait for one more approval before merging.

corinnekrych · 2019-03-28T09:36:29Z

make/test.mk

+	@docker login -u $(QUAY_USERNAME) -p $(QUAY_PASSWORD) $(REGISTRY_URI)
+	docker push $(DEVOPSCONSOLE_OPERATOR_REGISTRY_IMAGE):$(devopsconsole_version)
+
+	sed -e "s,REPLACE_IMAGE,$(DEVOPSCONSOLE_OPERATOR_REGISTRY_IMAGE):$(devopsconsole_version)," $(CUR_DIR)/test/olm/catalog_source.yaml | oc apply -f -


we need some if/else to keep Makefile macOS compatible. See we did with Tina did on previous test PR.

sbose78 · 2019-04-01T05:50:03Z

@baiju if that's a tiny change, you could. Otherwise we could merge this PR and consider doing that in a separate PR.
I would like to see how the usage with Openshift CI would be. Using the internal registry might be easier there - have to see how it goes.

baijum · 2019-04-01T05:55:24Z

@sbose78 That ConfigMap based approach requires some more work. Let's do it separately later.

Based on Len's PR: redhat-developer#56

sbose78 · 2019-04-01T09:51:23Z

/ok-to-test

sbose78 · 2019-04-01T14:15:18Z

make/test.mk

@@ -56,6 +56,32 @@ e2e-cleanup: get-test-namespace
 	$(Q)-oc delete -f ./deploy/test/role_binding_test.yaml --namespace $(TEST_NAMESPACE)
 	$(Q)-oc delete -f ./deploy/test/operator_test.yaml --namespace $(TEST_NAMESPACE)

+.PHONY: test-olm-integration
+## Runs the OLM integration tests without coverage
+test-olm-integration: push-operator-image olm-integration-setup


This calls

.PHONY: build-operator-image ## Build and create the operator container image build-operator-image: operator-sdk build $(DEVCONSOLE_OPERATOR_IMAGE)

The CI test pushing to the latest tag can be disastrous :)

Fixed. d27da00

sbose78 · 2019-04-01T14:22:43Z

On Mac

Step 5/7 : RUN sed -e "s,REPLACE_IMAGE,${image}," -i manifests/devconsole/${version}/devconsole-operator.v${version}.clusterserviceversion.yaml
 ---> Running in 5127469a3818
sed: couldn't open temporary file manifests/devconsole/0.1.0/sedZf4K3u: Permission denied
The command '/bin/sh -c sed -e "s,REPLACE_IMAGE,${image}," -i manifests/devconsole/${version}/devconsole-operator.v${version}.clusterserviceversion.yaml' returned a non-zero code: 4
make: *** [test-olm-integration] Error 4

baijum · 2019-04-01T15:37:28Z

@sbose78 Can you check the permission of the 0.1.0 directory in the host system (Mac)?

For me it shows something like this:

ls -ld manifests/devconsole/0.1.0/
drwxrwxr-x. 2 baiju baiju 4096 Apr  1 20:51 manifests/devconsole/0.1.0/

I just freshly cloned a new repo, I got the same permission.

sbose78 · 2019-04-01T16:07:18Z

$ ls -ld manifests/devconsole/0.1.0/
drwxr-xr-x  5 sbose  staff  160 Apr  1 12:01 manifests/devconsole/0.1.0/

sbose78 · 2019-04-01T16:31:19Z

On re-running:

oc apply -f ./test/e2e/subscription.yaml
subscription.operators.coreos.com/my-devopsconsole unchanged

Need to verify if we are cleaning up the subscription.

sbose78 · 2019-04-01T16:46:32Z

any_image(.)tag should be good as well.

sbose78 · 2019-04-01T16:50:00Z

The CatalogSource object gets created into olm namespace and the Subscription goes into operators . Let's ensure that the namespaces are present before we try creating resources in them. As far as I remember, the namespaces were absent in openshift 4.

sbose78 · 2019-04-01T17:41:46Z

.. although it would be fair to have this PR focus on openshift 3 with OLM, and then make this test work for openshift 4 in a different PR.

sbose78 · 2019-04-01T17:43:45Z

All version numbers can be a derivative of the commit id .

openshift-ci-robot · 2019-04-01T20:10:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Avni-Sharma, baijum, sbose78

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~~ [baijum,sbose78]

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

sbose78 · 2019-04-01T20:11:22Z

Following are the follow-up tasks:
#66
#65

Makefile script to install the operator and test

5a303e9

Refer: https://jira.coreos.com/browse/ODC-189

Avni-Sharma requested review from baijum, sbose78 and corinnekrych March 21, 2019 12:21

Avni-Sharma requested a review from ldimaggi March 21, 2019 12:25

baijum requested a review from kwk March 21, 2019 13:08

sbose78 reviewed Mar 21, 2019

View reviewed changes

kwk reviewed Mar 21, 2019

View reviewed changes

Avni-Sharma added 2 commits March 22, 2019 18:23

Added go test file for component CR

d54913c

Following the operator-sdk testing framework. Referred Corrine's/Tina's PR.

Merge remote-tracking branch 'refs/remotes/upstream/master' into olm_…

6bc6f68

…makefile

sbose78 reviewed Mar 22, 2019

View reviewed changes

baijum changed the title ~~Makefile script to install the operator and test~~ Create Makefile target to install operator & test Mar 26, 2019

baijum added 2 commits March 27, 2019 04:59

Merge remote-tracking branch 'upstream/master' into olm_makefile

0e820ab

Cleanup Makefile

326d49d

baijum reviewed Mar 27, 2019

View reviewed changes

baijum added 3 commits March 27, 2019 10:23

more assertions

2eed983

Merge remote-tracking branch 'upstream/master' into olm_makefile

0cae897

Assert status

686533c

baijum marked this pull request as ready for review March 27, 2019 06:52

Avni-Sharma requested a review from tinakurian March 27, 2019 08:33

Merge remote-tracking branch 'upstream/master' into olm_makefile

517cf7f

baijum approved these changes Mar 28, 2019

View reviewed changes

openshift-ci-robot added the approved label Mar 28, 2019

baijum added 2 commits March 28, 2019 11:20

Merge remote-tracking branch 'upstream/master' into olm_makefile

29d6e20

Pass image & version as arguments

5fe0b3c

corinnekrych reviewed Mar 28, 2019

View reviewed changes

baijum added 2 commits April 1, 2019 08:37

Fix name to run the test

6263552

Remove unused variable

ec13aa8

Avni-Sharma and others added 2 commits April 1, 2019 12:08

Adding Git Hash to the opertaor registry image

2458c1f

Assert for mata info (commented)

3edc000

Based on Len's PR: redhat-developer#56

openshift-ci-robot added the ok-to-test label Apr 1, 2019

Avni-Sharma added 4 commits April 1, 2019 15:48

Adding Git hash and changed devops to dev in component_test.go

d47b270

changed devops to dev

e03caba

Merge remote-tracking branch 'origin/olm_makefile' into olm_makefile

e99a07f

Merge remote-tracking branch 'upstream/master' into olm_makefile

af95f76

sbose78 reviewed Apr 1, 2019

View reviewed changes

baijum added 2 commits April 1, 2019 20:51

Merge remote-tracking branch 'upstream/master' into olm_makefile

82480eb

include tag in build

d27da00

Use "dev" instead of "devops"

b16e650

sbose78 mentioned this pull request Apr 1, 2019

Consider cleaning up catalogSource, Subscription #65

Closed

sbose78 approved these changes Apr 1, 2019

View reviewed changes

sbose78 merged commit 7f3e8a4 into redhat-developer:master Apr 1, 2019

baijum mentioned this pull request Apr 2, 2019

Fix e2e local #67

Merged


		operator-sdk test local ./test/olm/ --go-test-flags "-v -parallel=1"

		#TODO: the below command should be replaced with the actual test command.

Create Makefile target to install operator & test #33

Create Makefile target to install operator & test #33

Conversation

Avni-Sharma commented Mar 21, 2019 • edited by baijum Loading

centos-ci commented Mar 21, 2019

Avni-Sharma commented Mar 21, 2019

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

sbose78 commented Mar 22, 2019

sbose78 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

sbose78 commented Mar 27, 2019 • edited Loading

baijum left a comment

Choose a reason for hiding this comment

corinnekrych Mar 28, 2019 • edited Loading

Choose a reason for hiding this comment

sbose78 commented Apr 1, 2019

baijum commented Apr 1, 2019

sbose78 commented Apr 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Apr 1, 2019

baijum commented Apr 1, 2019

sbose78 commented Apr 1, 2019

sbose78 commented Apr 1, 2019

sbose78 commented Apr 1, 2019

sbose78 commented Apr 1, 2019

sbose78 commented Apr 1, 2019

sbose78 commented Apr 1, 2019 • edited Loading

openshift-ci-robot commented Apr 1, 2019

sbose78 commented Apr 1, 2019

Avni-Sharma commented Mar 21, 2019 •

edited by baijum

Loading

sbose78 commented Mar 27, 2019 •

edited

Loading

corinnekrych Mar 28, 2019 •

edited

Loading

sbose78 commented Apr 1, 2019 •

edited

Loading