Makefile setup, test stubs #3

michaelkleinhenz · 2019-02-14T12:50:02Z

This adds a Makefile setup and stubs for unit and e2e tests. Execute make to get a list of possible targets. This was essentially taken from toolchain-operator with added test setup.

kwk

I bet 90% of the copy'n'pasted Makefile stuff is not used and therefore shouldn't be added. I would rather start fresh. Especially the test.mk file is way to heavy and shouldn't be copied 1:1.

kwk · 2019-02-14T13:49:44Z

Dockerfile.builder

@@ -0,0 +1,37 @@
+FROM centos:7


Instead of having two Dockerfiles, we should start using multi-staged Dockerfiles.

Makefile

cmd/manager/main.go

michaelkleinhenz · 2019-02-14T15:15:32Z

@kwk no, all of the code is used/tested/inspected with the exception of the coverage and the windows stuff. There is not much overhead and I have gone over the make process line-by-line. @dipak-pawar did a great job there.

alexeykazakov · 2019-02-14T19:58:22Z

Gopkg.lock

  revision = "de5bf2ad457846296e2031421a34e2568e304e35"

 [[projects]]
-  digest = "1:4b8b5811da6970495e04d1f4e98bb89518cc3cfc3b3f456bdb876ed7b6c74049"
+  branch = "master"
+  name = "github.com/alexeykazakov/devconsole"


We need to rename all github.com/alexeykazakov/devconsole packages :) In a separate PR.

Yeah, rebased. There are also some base image references in the dockerfiles that need updating once we know the repository address.

sbose78 · 2019-02-14T23:15:34Z

Opened a PR to make the code build #4

kwk · 2019-02-15T08:49:15Z

@kwk no, all of the code is used/tested/inspected with the exception of the coverage and the windows stuff. There is not much overhead and I have gone over the make process line-by-line. @dipak-pawar did a great job there.

That is my point, the coverage stuff takes up 90% of the code and can be simplified/removed like crazy.

…nor fixes.

michaelkleinhenz · 2019-02-15T10:18:56Z

@kwk Coverage is now also tested and working. Removed/updated a few things, also removed windows support. 98d2b3f.

deploy/test/global-manifests.yaml

dipak-pawar · 2019-02-15T10:24:40Z

Dockerfile.deploy

+LABEL maintainer "Devtools <devtools@redhat.com>"
+LABEL author "Devtools <devtools@redhat.com>"
+ENV LANG=en_US.utf8
+ENV F8_INSTALL_PREFIX=/usr/local/devconsole-operator


maybe let's get rid out of F8 prefix

Fixed befe3ef.

dipak-pawar · 2019-02-15T10:24:50Z

Dockerfile.deploy

+ENV F8_INSTALL_PREFIX=/usr/local/devconsole-operator
+
+# Create a non-root user and a group with the same name: "devconsole-operator"
+ENV F8_USER_NAME=devconsole-operator


maybe let's get rid out of F8 prefix

Fixed befe3ef.

@michaelkleinhenz no it wasn't fixed in befe3ef. The code is still in there. We have F8_ in various other locations and files as well if you take a look at the PR now. I suggest to keep the F8_ and not replace it now. The reason is that environment variables marked with F8_ are those that you should care about setting from the outside when running a Makefile for example: F8_POSTGRES_HOST=foo make test-integration. Can you revert the parts of befe3ef that modify the F8_ bits please and keep the rest until we have a proper replacement for F8_?

Makefile

dipak-pawar · 2019-02-15T10:28:05Z

Makefile

+REGISTRY_IMAGE = ${PROJECT_NAME}
+
+ifeq ($(TARGET),rhel)
+	REGISTRY_URL := ${REGISTRY_URI}/openshiftio/rhel-${REGISTRY_NS}-${REGISTRY_IMAGE}


can we change docker org as we are starting fresh?

Same as below. Let's do this when we know how it will look like.

dipak-pawar · 2019-02-15T10:29:30Z

make/Makefile.dev

@@ -0,0 +1,89 @@
+DOCKER_REPO?=quay.io/openshiftio


can we change this docker org?

Yes, let's do this when we know what it is..

make/Makefile.dev

dipak-pawar · 2019-02-15T10:31:32Z

make/test.mk

+.PHONY: e2e-cleanup
+e2e-cleanup:
+	oc login -u system:admin
+	oc delete oauthclient codeready-devconsole || true


this can be removed

Fixed befe3ef.

make/test.mk

test/README.md

alexeykazakov

Let's move on with that change :) We can improve it later. But it's IMO a good start and worth merging now so we can simplify local development a lot.

kwk

@michaelkleinhenz I went through all commits and resolved all conversations that you addressed. That leaves a few open on which I would like to ask that you comment on them. @alexeykazakov Yes we can improve later but let's fix this tiny F8_ bit first, okay? Shouldn't cost much time.

kwk · 2019-02-22T06:51:25Z

Dockerfile.deploy

+ENV F8_INSTALL_PREFIX=/usr/local/devconsole-operator
+
+# Create a non-root user and a group with the same name: "devconsole-operator"
+ENV F8_USER_NAME=devconsole-operator


@michaelkleinhenz no it wasn't fixed in befe3ef. The code is still in there. We have F8_ in various other locations and files as well if you take a look at the PR now. I suggest to keep the F8_ and not replace it now. The reason is that environment variables marked with F8_ are those that you should care about setting from the outside when running a Makefile for example: F8_POSTGRES_HOST=foo make test-integration. Can you revert the parts of befe3ef that modify the F8_ bits please and keep the rest until we have a proper replacement for F8_?

alexeykazakov · 2019-02-25T19:36:58Z

@michaelkleinhenz any updates on that PR?

corinnekrych

Agreedwith alexey, let's merge this initial version we can improve afterwards.

Not dismissing, let's address it in another PR.

* Added stub tests and makefile setup. * Exposed more targets, removed windows make code, checked coverage, minor fixes.

Added stub tests and makefile setup.

65df4cc

kwk suggested changes Feb 14, 2019

View reviewed changes

alexeykazakov reviewed Feb 14, 2019

View reviewed changes

michaelkleinhenz added 2 commits February 15, 2019 10:46

Merge remote-tracking branch 'upstream/master' into test-makefile-stubs

535f812

Exposed more targets, removed windows make code, checked coverage, mi…

98d2b3f

…nor fixes.

dipak-pawar reviewed Feb 15, 2019

View reviewed changes

deploy/test/global-manifests.yaml Outdated Show resolved Hide resolved

deploy/test/global-manifests.yaml Outdated Show resolved Hide resolved

deploy/test/global-manifests.yaml Outdated Show resolved Hide resolved

dipak-pawar reviewed Feb 15, 2019

View reviewed changes

Fixed review comments.

befe3ef

alexeykazakov approved these changes Feb 22, 2019

View reviewed changes

kwk previously requested changes Feb 22, 2019

View reviewed changes

corinnekrych approved these changes Feb 28, 2019

View reviewed changes

sbose78 approved these changes Feb 28, 2019

View reviewed changes

sbose78 merged commit f130073 into redhat-developer:master Feb 28, 2019

sbose78 mentioned this pull request Feb 28, 2019

fix environment variable prefix #8

Closed

corinnekrych pushed a commit to corinnekrych/devopsconsole-operator that referenced this pull request Mar 4, 2019

Makefile setup, test stubs (redhat-developer#3)

a69ee1b

* Added stub tests and makefile setup. * Exposed more targets, removed windows make code, checked coverage, minor fixes.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile setup, test stubs #3

Makefile setup, test stubs #3

michaelkleinhenz commented Feb 14, 2019

kwk left a comment

kwk Feb 14, 2019

michaelkleinhenz commented Feb 14, 2019

alexeykazakov Feb 14, 2019

michaelkleinhenz Feb 15, 2019

sbose78 commented Feb 14, 2019

kwk commented Feb 15, 2019

michaelkleinhenz commented Feb 15, 2019

dipak-pawar Feb 15, 2019

michaelkleinhenz Feb 15, 2019

dipak-pawar Feb 15, 2019

michaelkleinhenz Feb 15, 2019

kwk Feb 22, 2019

dipak-pawar Feb 15, 2019

michaelkleinhenz Feb 15, 2019

dipak-pawar Feb 15, 2019

michaelkleinhenz Feb 15, 2019

dipak-pawar Feb 15, 2019

michaelkleinhenz Feb 15, 2019

alexeykazakov left a comment

kwk left a comment

kwk Feb 22, 2019

alexeykazakov commented Feb 25, 2019

corinnekrych left a comment

Makefile setup, test stubs #3

Makefile setup, test stubs #3

Conversation

michaelkleinhenz commented Feb 14, 2019

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelkleinhenz commented Feb 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 commented Feb 14, 2019

kwk commented Feb 15, 2019

michaelkleinhenz commented Feb 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov left a comment

Choose a reason for hiding this comment

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeykazakov commented Feb 25, 2019

corinnekrych left a comment

Choose a reason for hiding this comment