change makefile test target for no longer be required manual steps to run the commands #3983

camilamacedo86 · 2020-10-06T14:34:33Z

Description of the change:
Update the Makefile test target as discussed in the demo.

Motivation for the change:
Closes: #3692

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

Local Tests

 $ operator-sdk init 
Writing scaffold for you to edit...
Get controller runtime:
$ go get sigs.k8s.io/controller-runtime@v0.6.2
Update go.mod:
$ go mod tidy
Running make:
$ make
/Users/camilamacedo/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
go build -o bin/manager main.go
Next: define a resource with:
$ operator-sdk create api
camilamacedo@Camilas-MacBook-Pro ~/go/src/testmake (master) $ cat Makefile 
# Current Operator version
VERSION ?= 0.0.1
# Default bundle image tag
BUNDLE_IMG ?= controller-bundle:$(VERSION)
# Options for 'bundle-build'
ifneq ($(origin CHANNELS), undefined)
BUNDLE_CHANNELS := --channels=$(CHANNELS)
endif
ifneq ($(origin DEFAULT_CHANNEL), undefined)
BUNDLE_DEFAULT_CHANNEL := --default-channel=$(DEFAULT_CHANNEL)
endif
BUNDLE_METADATA_OPTS ?= $(BUNDLE_CHANNELS) $(BUNDLE_DEFAULT_CHANNEL)

# Image URL to use all building/pushing image targets
IMG ?= controller:latest
# Produce CRDs that work back to Kubernetes 1.11 (no version conversion)
CRD_OPTIONS ?= "crd:trivialVersions=true"

# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
ifeq (,$(shell go env GOBIN))
GOBIN=$(shell go env GOPATH)/bin
else
GOBIN=$(shell go env GOBIN)
endif

all: manager

# Run tests
ENVTEST_ASSETS_DIR=$(shell pwd)/testbin
test: generate fmt vet manifests
	mkdir -p ${ENVTEST_ASSETS_DIR}
	test -f ${ENVTEST_ASSETS_DIR}/setup-envtest.sh || curl -sSLo ${ENVTEST_ASSETS_DIR}/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/master/hack/setup-envtest.sh
	source ${ENVTEST_ASSETS_DIR}/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out

...

And to check that is running:

$ make test
/Users/camilamacedo/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
/Users/camilamacedo/go/bin/controller-gen "crd:trivialVersions=true" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
mkdir -p /Users/camilamacedo/go/src/testmake/testbin
test -f /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh || curl -sSLo /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/master/hack/setup-envtest.sh
source /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh; fetch_envtest_tools /Users/camilamacedo/go/src/testmake/testbin; setup_envtest_env /Users/camilamacedo/go/src/testmake/testbin; go test ./... -coverprofile cover.out
fetching envtest tools@1.16.4 (into '/Users/camilamacedo/go/src/testmake/testbin')
x bin/
x bin/etcd
x bin/kubectl
x bin/kube-apiserver
setting up env vars
?   	testmake	[no test files]
camilamacedo@Camilas-MacBook-Pro ~/go/src/testmake (master) $

changelog/fragments/00-template.yaml

internal/plugins/manifests/init.go

changelog/fragments/00-template.yaml

camilamacedo86 · 2020-10-06T15:17:02Z

Hi @joelanford,

Tks for your review the changes are made. 👍 See that it is working:

$ make test
/Users/camilamacedo/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
/Users/camilamacedo/go/bin/controller-gen "crd:trivialVersions=true" rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
mkdir -p /Users/camilamacedo/go/src/testmake/testbin
test -f /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh || curl -sSLo /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/master/hack/setup-envtest.sh
source /Users/camilamacedo/go/src/testmake/testbin/setup-envtest.sh; fetch_envtest_tools /Users/camilamacedo/go/src/testmake/testbin; setup_envtest_env /Users/camilamacedo/go/src/testmake/testbin; go test ./... -coverprofile cover.out
fetching envtest tools@1.16.4 (into '/Users/camilamacedo/go/src/testmake/testbin')
x bin/
x bin/etcd
x bin/kubectl
x bin/kube-apiserver
setting up env vars
?   	testmake	[no test files]
camilamacedo@Camilas-MacBook-Pro ~/go/src/testmake (master) $

estroz

This Makefile addition is unrelated to the manifests plugin, so it doesn't make sense to me to add it here. It should be added directly to the Go Makefile template, and it was in kubernetes-sigs/kubebuilder#1626 for go.kubebuilder.io/v3-alpha. So the right solution is to support the v3-alpha plugin in the operator-sdk binary so users can opt-in; this is the solution we discussed during triage.

joelanford · 2020-10-06T16:47:41Z

@estroz Do you think there's a path for us to make the default scaffolding work with the Kubebuilder v2 plugin?

I think the problem we'll find with the v3-alpha plugin is that no one will use it until it is the default.

estroz · 2020-10-06T17:08:01Z

Yes, since this is a non-breaking change I think kubernetes-sigs/kubebuilder#1626 can be backported to the v2 plugin. @camilamacedo86 can you create a PR for this upstream?

camilamacedo86 · 2020-10-06T18:16:37Z

Hi @estroz,

I agree that in this case, we might able to push to v2+ as well. Could you please check the PR: kubernetes-sigs/kubebuilder#1711 for we are able to bump the kb commit here?

camilamacedo86 · 2020-10-12T18:44:12Z

HI @joelanford @estroz,
Based on the discussion in the bug triage meeting could you please let us know if we can get it to merge or fi has anything else that you would like o change before that?

estroz · 2020-10-12T19:06:29Z

internal/plugins/manifests/init.go

@@ -18,12 +18,17 @@ package manifests
 import (


These changes shouldn't be in the manifests plugin, they should be in their own internal/plugins/envtest plugin.

internal/plugins/manifests/init.go

jberkhahn · 2020-10-12T19:21:47Z

can confirm that this works, but I agree with @estroz that it should be in its own file, and then we can just delete it when the upstream fix gets merged

… run the commands

estroz

Some nits, otherwise lgtm

estroz · 2020-10-13T19:58:50Z

internal/plugins/envtest/init.go

+// ControllerRuntimeEnvTestVersion version to be used to download the envtest setup script
+// todo: update the tag release when the next version of the project be released with this script
+const controllerRuntimeEnvTestVersion = "v0.6.3"


Suggested change

// ControllerRuntimeEnvTestVersion version to be used to download the envtest setup script

// todo: update the tag release when the next version of the project be released with this script

const controllerRuntimeEnvTestVersion = "v0.6.3"

// controllerRuntimeVersion version to be used to download the envtest setup script

// todo: update the tag release when the next version of the project be released with this script

const controllerRuntimeVersion = "v0.6.3"

estroz · 2020-10-13T19:59:24Z

internal/plugins/envtest/init.go

+
+	makefileBytes = []byte(strings.Replace(string(makefileBytes),
+		"# Run tests\ntest: generate fmt vet manifests\n\tgo test ./... -coverprofile cover.out",
+		fmt.Sprintf(makefileTestTarget, controllerRuntimeEnvTestVersion), 1))


Suggested change

fmt.Sprintf(makefileTestTarget, controllerRuntimeEnvTestVersion), 1))

fmt.Sprintf(makefileTestTarget, controllerRuntimeVersion), 1))

estroz · 2020-10-13T20:02:09Z

internal/plugins/envtest/init.go

+ENVTEST_ASSETS_DIR=$(shell pwd)/testbin
+test: generate fmt vet manifests
+	mkdir -p ${ENVTEST_ASSETS_DIR}
+	test -f ${ENVTEST_ASSETS_DIR}/setup-envtest.sh || curl -sSLo ${ENVTEST_ASSETS_DIR}/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/%s/hack/setup-envtest.sh
+	source ${ENVTEST_ASSETS_DIR}/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out`


Consistency nit:

Suggested change

ENVTEST_ASSETS_DIR=$(shell pwd)/testbin

test: generate fmt vet manifests

mkdir -p ${ENVTEST_ASSETS_DIR}

test -f ${ENVTEST_ASSETS_DIR}/setup-envtest.sh || curl -sSLo ${ENVTEST_ASSETS_DIR}/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/%s/hack/setup-envtest.sh

source ${ENVTEST_ASSETS_DIR}/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out`

ENVTEST_ASSETS_DIR = $(shell pwd)/testbin

test: generate fmt vet manifests

mkdir -p $(ENVTEST_ASSETS_DIR)

test -f $(ENVTEST_ASSETS_DIR)/setup-envtest.sh || curl -sSLo $(ENVTEST_ASSETS_DIR)/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/%s/hack/setup-envtest.sh

source $(ENVTEST_ASSETS_DIR)/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR); go test ./... -coverprofile cover.out`

estroz · 2020-10-13T20:02:36Z

internal/plugins/envtest/init.go

+// ControllerRuntimeEnvTestVersion version to be used to download the envtest setup script
+const controllerRuntimeEnvTestVersion = "v0.6.3"


Suggested change

// ControllerRuntimeEnvTestVersion version to be used to download the envtest setup script

const controllerRuntimeEnvTestVersion = "v0.6.3"

// controllerRuntimeVersion version to be used to download the envtest setup script

const controllerRuntimeVersion = "v0.6.3"

estroz · 2020-10-13T20:05:01Z

internal/plugins/envtest/init.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// TODO: it will no longer required when the default plugin be v3+


Isn't this going into v2?

Not decided and merged to v2 yet. We might clarify it.

jberkhahn · 2020-10-13T20:06:26Z

/lgtm

joelanford

A couple of nits.

joelanford · 2020-10-13T20:49:43Z

changelog/fragments/makefile_test.yaml

+      Change the makefile target test for no longer be required manually changes and
+      steps to setup the envtest.


Suggested change

Change the makefile target test for no longer be required manually changes and

steps to setup the envtest.

In Go projects, resolved an issue that caused failing tests by changing the Makefile's `test` target to automatically download and configure the necessary `envtest` binaries.

joelanford · 2020-10-13T20:52:09Z

internal/plugins/golang/v2/init.go

+	"github.com/operator-framework/operator-sdk/internal/plugins/envtest"
+


This should be grouped with the other SDK imports.

internal/plugins/envtest/init.go

joelanford

/lgtm

After addressing Eric's filemode suggestion.

openshift-ci-robot · 2020-10-14T16:25:25Z

New changes are detected. LGTM label has been removed.

estroz · 2020-10-14T19:49:22Z

hack/tests/sanity-check.sh

@@ -14,4 +14,4 @@ make install
 go run ./hack/generate/samples/generate_all.go

 # Make sure repo is still in a clean state.
-git diff --exit-code
+git diff --exit-code -- . ':!testdata/go/memcached-operator/bin/*'


This can be removed for now. We can address in a follow-up.

estroz · 2020-10-14T19:49:26Z

.gitignore

@@ -110,3 +110,7 @@ website/public/
 website/resources/
 website/node_modules/
 website/tech-doc-hugo
+
+# samples bin
+testdata/go/memcached-operator/bin/*


This can be removed for now. We can address in a follow-up.

IHMO we should keep in order to ensure that the bin will not be committed never and ever,

hack/tests/sanity-check.sh

asmacdo · 2020-10-14T21:11:07Z

The Ansible test is failing. RUN ansible-galaxy collection install -r ${HOME}/requirements.yml is timing out. This times out for me locally, and others are seeing this as well. Since Ansible galaxy appears to be down, please do not restart that job for a while.

… run the commands operator-framework#3983

… run the commands #3983 (#4038) Cherry-pick #3983

The test make target required some preparations,specifically etcd was not found in the expected location. This adds the changes from operator-framework/operator-sdk#3983 to the test target to make it autonomous. Signed-off-by: Michael Adam <obnox@redhat.com>

openshift-ci-robot requested review from rashmigottipati and theishshah October 6, 2020 14:34

camilamacedo86 mentioned this pull request Oct 6, 2020

doc: make implicity the users must set envtest #3709

Closed

joelanford requested changes Oct 6, 2020

View reviewed changes

changelog/fragments/00-template.yaml Outdated Show resolved Hide resolved

internal/plugins/manifests/init.go Outdated Show resolved Hide resolved

changelog/fragments/00-template.yaml Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the makefile-test-update branch from 0787bc8 to c4e3b7b Compare October 6, 2020 15:16

camilamacedo86 requested a review from joelanford October 6, 2020 15:23

estroz suggested changes Oct 6, 2020

View reviewed changes

camilamacedo86 changed the title ~~change makefile test target for no longer be required manual steps to run the commands~~ WIP: change makefile test target for no longer be required manual steps to run the commands Oct 6, 2020

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 6, 2020

jberkhahn mentioned this pull request Oct 12, 2020

Makefile should not need to be modified to get tests to run #3692

Closed

jberkhahn removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2020

camilamacedo86 changed the title ~~WIP: change makefile test target for no longer be required manual steps to run the commands~~ change makefile test target for no longer be required manual steps to run the commands Oct 12, 2020

estroz suggested changes Oct 12, 2020

View reviewed changes

camilamacedo86 force-pushed the makefile-test-update branch 4 times, most recently from 4488fb4 to 45340d9 Compare October 13, 2020 19:51

change makefile test target for no longer be required manual steps to…

a2c962b

… run the commands

camilamacedo86 force-pushed the makefile-test-update branch from 45340d9 to a2c962b Compare October 13, 2020 19:59

estroz suggested changes Oct 13, 2020

View reviewed changes

openshift-ci-robot assigned jberkhahn Oct 13, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 13, 2020

joelanford reviewed Oct 13, 2020

View reviewed changes

camilamacedo86 mentioned this pull request Oct 14, 2020

Release v1.1.0 #4031

Merged

estroz approved these changes Oct 14, 2020

View reviewed changes

internal/plugins/envtest/init.go Outdated Show resolved Hide resolved

joelanford approved these changes Oct 14, 2020

View reviewed changes

openshift-ci-robot assigned joelanford Oct 14, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020

fix import suggestion

46197c0

camilamacedo86 force-pushed the makefile-test-update branch from 34d586a to 46197c0 Compare October 14, 2020 18:22

Camila Macedo added 2 commits October 14, 2020 15:53

ignory'

af24603

fix sanity

ba5768a

estroz reviewed Oct 14, 2020

View reviewed changes

camilamacedo86 commented Oct 14, 2020

View reviewed changes

hack/tests/sanity-check.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

94e4c4f

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020

Merge branch 'master' into makefile-test-update

02b267d

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2020

camilamacedo86 merged commit c36f1b6 into operator-framework:master Oct 14, 2020

camilamacedo86 deleted the makefile-test-update branch October 14, 2020 23:08

camilamacedo86 added a commit to camilamacedo86/operator-sdk that referenced this pull request Oct 15, 2020

change makefile test target for no longer be required manual steps to…

b3a6a9c

… run the commands operator-framework#3983

camilamacedo86 mentioned this pull request Oct 15, 2020

change makefile test target for no longer be required manual steps to… #4038

Merged

camilamacedo86 added a commit to camilamacedo86/operator-sdk that referenced this pull request Oct 15, 2020

change makefile test target for no longer be required manual steps to…

5786c2d

… run the commands operator-framework#3983

jberkhahn mentioned this pull request Oct 15, 2020

Remove our changes to kubebuilder generated makefiles #4044

Closed

camilamacedo86 added a commit that referenced this pull request Oct 19, 2020

change makefile test target for no longer be required manual steps to…

ba9227c

… run the commands #3983 (#4038) Cherry-pick #3983

anmol372 mentioned this pull request Nov 10, 2020

[docs] Improve Website Styling #4004

Merged

2 tasks

obnoxxx mentioned this pull request Feb 8, 2021

Make make test work samba-in-kubernetes/samba-operator#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change makefile test target for no longer be required manual steps to run the commands #3983

change makefile test target for no longer be required manual steps to run the commands #3983

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

estroz left a comment •

edited

Loading

joelanford commented Oct 6, 2020

estroz commented Oct 6, 2020

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

camilamacedo86 commented Oct 12, 2020 •

edited

Loading

estroz Oct 12, 2020

jberkhahn commented Oct 12, 2020

estroz left a comment

estroz Oct 13, 2020

estroz Oct 13, 2020

estroz Oct 13, 2020

estroz Oct 13, 2020

estroz Oct 13, 2020

camilamacedo86 Oct 14, 2020

jberkhahn commented Oct 13, 2020

joelanford left a comment

joelanford Oct 13, 2020

joelanford Oct 13, 2020

joelanford left a comment

openshift-ci-robot commented Oct 14, 2020

estroz Oct 14, 2020

camilamacedo86 Oct 14, 2020

estroz Oct 14, 2020

camilamacedo86 Oct 14, 2020

asmacdo commented Oct 14, 2020

	fmt.Sprintf(makefileTestTarget, controllerRuntimeEnvTestVersion), 1))
	fmt.Sprintf(makefileTestTarget, controllerRuntimeVersion), 1))

		// ControllerRuntimeEnvTestVersion version to be used to download the envtest setup script
		const controllerRuntimeEnvTestVersion = "v0.6.3"

		Change the makefile target test for no longer be required manually changes and
		steps to setup the envtest.

	Change the makefile target test for no longer be required manually changes and
	steps to setup the envtest.
	In Go projects, resolved an issue that caused failing tests by changing the Makefile's `test` target to automatically download and configure the necessary `envtest` binaries.

		"github.com/operator-framework/operator-sdk/internal/plugins/envtest"

change makefile test target for no longer be required manual steps to run the commands #3983

change makefile test target for no longer be required manual steps to run the commands #3983

Conversation

camilamacedo86 commented Oct 6, 2020 • edited Loading

camilamacedo86 commented Oct 6, 2020 • edited Loading

estroz left a comment • edited Loading

Choose a reason for hiding this comment

joelanford commented Oct 6, 2020

estroz commented Oct 6, 2020

camilamacedo86 commented Oct 6, 2020 • edited Loading

camilamacedo86 commented Oct 12, 2020 • edited Loading

Choose a reason for hiding this comment

jberkhahn commented Oct 12, 2020

estroz 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

jberkhahn commented Oct 13, 2020

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 14, 2020

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

asmacdo commented Oct 14, 2020

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

estroz left a comment •

edited

Loading

camilamacedo86 commented Oct 6, 2020 •

edited

Loading

camilamacedo86 commented Oct 12, 2020 •

edited

Loading