*: refactor Dockerfiles and Makefiles #4069

estroz · 2020-10-20T00:11:10Z

Description of the change:

*: move make targets around such that release-related targets are in their own Makefile, ex. image builds are in release/Makefile
images/: move all Dockerfiles from hack/image to here, update references
hack/: remove unnecessary wrapper scripts

Motivation for the change: every make rule used for development purposes should exist in the top-level Makefile; every rule related to releases should exist in release/Makefile to reduce clutter and separate concerns. Dockerfiles have been consolidated into images/ and slimmed down for similar reasons, including removing unnecessary docker build wrapper scripts. These images can be built by running make image/<image base name> for development purposes.

Nothing developer-facing has changed except for image build targets (image-build-<base name> -> image/<base name>) and multi-arch build targets (similarly renamed, and moved to release/Makefile).

Relates to upcoming release automation work (see #3939, #4034).

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

their own Makefile, ex. image builds are in release/Makefile images/: move all Dockerfiles from hack/image to here, update references hack/: remove unnecessary wrapper scripts

tengqm · 2020-10-20T01:36:48Z

Is the following a hint that our directory hierarchy needs some simplification? I'm afraid the paths for the files are too long.

Makefile

images/operator-sdk/Dockerfile

release/Makefile

jmrodri · 2020-10-20T14:16:48Z

The release directory will likely affect the next downstream release since we already have a release directory downstream: https://github.com/openshift/ocp-release-operator-sdk/tree/master/release

jmrodri · 2020-10-20T16:22:33Z

/hold
until I can finish reviewing

jmrodri

My only real concern in the short term is the release directory will probably cause a conflict and interact with the one in the downstream repo. This re-introduces the image/* targets again which were removed in favor of - target names. Technically I don't see anything wrong here so I'm okay with it.

Makefile

jmrodri · 2020-10-20T17:05:55Z

release/Makefile

+		echo "git and/or gpg are not configured to have default signing key $${default_key}"; \
+		exit 1; \
+	fi; \
+	}


nit: would be helpful to have the help target in the makefile, but I suspect we shouldn't be interacting with this file directly. I did find myself going into release and running make help to see what was in it :)

.PHONY: help help: ## Show this help screen. @echo 'Usage: make <OPTIONS> ... <TARGETS>' @echo '' @echo 'Available targets are:' @echo '' @awk 'BEGIN {FS = ":.*##"; printf "\nUsage:\n make \033[36m<target>\033[0m\n"} /^[a-zA-Z0-9_-]+:.*?##/ { printf " \033[36m%-25s\033[0m %s\n", $$1, $$2 } /^##@/ { printf "\n\033[1m%s\033[0m\n", substr($$0, 5) } ' $(MAKEFILE_LIST)

I'll add this in #4034 since I'd like that Makefile to be called directly.

+1 for the suggestion

jmrodri · 2020-10-20T17:43:38Z

Makefile

-$(e2e_targets):: test-e2e-setup image-build-scorecard-test
+$(e2e_targets):: test-e2e-setup image/scorecard-test

 test-e2e:: $(e2e_tests) ## Run e2e tests
-test-e2e-go:: image-build-custom-scorecard-tests ## Run Go e2e tests
+test-e2e-go:: image/custom-scorecard-tests ## Run Go e2e tests
 	go test ./test/e2e-go -v -ginkgo.v
-test-e2e-ansible:: image-build-ansible ## Run Ansible e2e tests
+test-e2e-ansible:: image/ansible-operator ## Run Ansible e2e tests
 	go test -count=1 ./internal/ansible/proxy/...
 	go test ./test/e2e-ansible -v -ginkgo.v
-test-e2e-ansible-molecule:: image-build-ansible ## Run molecule-based Ansible e2e tests
+test-e2e-ansible-molecule:: image/ansible-operator ## Run molecule-based Ansible e2e tests
 	./hack/tests/e2e-ansible-molecule.sh
-test-e2e-helm:: image-build-helm ## Run Helm e2e tests
+test-e2e-helm:: image/helm-operator ## Run Helm e2e tests


I actually removed all the slashes from the image builds last year, in PRs #2019 & #2157. The slash method does reduce the duplication. I guess it's okay, I just find all the "reusable" targets hard to grep and figure out initially. I know about them now.

Yeah the only reason I re-added <type>/<pattern> is for re-usability and de-cluttering, since Makefiles have great pattern matching/substitution functionality.

jmrodri · 2020-10-20T17:46:13Z

/hold cancel

camilamacedo86 · 2020-10-20T20:35:44Z

images/scorecard-test-kuttl/Dockerfile

    KUBECONFIG=

-# install test binary 
-COPY scorecard-test-kuttl ${TESTKUTTL}
+RUN echo "${USER_NAME}:x:${USER_UID}:0:${USER_NAME} user:${HOME}:/sbin/nologin" >> /etc/passwd


Now we have all images following the same standard.

camilamacedo86 · 2020-10-20T20:45:58Z

Makefile

@@ -41,79 +45,39 @@ fix: ## Fixup files in the repo.

 .PHONY: clean
 clean: ## Cleanup build artifacts and tool binaries.
-	rm -rf $(BUILD_DIR) dist tools/bin


should we not ignore the tools/ dir ?

tools/bin is already ignored. We can't ignore the whole tools/ directory since it contains scripts.

joelanford

A couple more comments and questions. Also, do you have a CI run in travis you can link that includes the deploy phases?

.travis.yml

Makefile

release/Makefile

camilamacedo86

I am ok with these changes when passing in the CI.

Also, I added a few nits.

I will add the nits

camilamacedo86

I executed make release from root dir and then, it created the bins into the dist/, however:

The checks made before was not made. E.g ensure that has a tag, do not allow uncommitted files.

 $ make release
rm -rf build dist tools/bin
make -f release/Makefile GO_BUILD_ARGS='-gcflags "all=-trimpath=/Users/camilamacedo/go/src/github.com/operator-framework" -asmflags "all=-trimpath=/Users/camilamacedo/go/src/github.com/operator-framework" -ldflags " -X 'github.com/operator-framework/operator-sdk/internal/version.Version=v1.1.0+git' -X 'github.com/operator-framework/operator-sdk/internal/version.GitVersion=v1.1.0-7-gd7e63051' -X 'github.com/operator-framework/operator-sdk/internal/version.GitCommit=d7e63051b1dd74e0da8b88ab8de331e3e4af1348' -X 'github.com/operator-framework/operator-sdk/internal/version.KubernetesVersion=v1.18.8' " '
make[1]: Entering directory '/Users/camilamacedo/go/src/github.com/operator-framework/operator-sdk'
{ \
cmdpkg=$(echo operator-sdk-v1.1.0-7-gd7e63051-aarch64-linux-gnu | sed -E "s/(operator-sdk|ansible-operator|helm-operator).*/\1/"); \
go build -gcflags "all=-trimpath=/Users/camilamacedo/go/src/github.com/operator-framework" -asmflags "all=-trimpath=/Users/camilamacedo/go/src/github.com/operator-framework" -ldflags " -X github.com/operator-framework/operator-sdk/internal/version.Version=v1.1.0+git -X github.com/operator-framework/operator-sdk/internal/version.GitVersion=v1.1.0-7-gd7e63051 -X github.com/operator-framework/operator-sdk/internal/version.GitCommit=d7e63051b1dd74e0da8b88ab8de331e3e4af1348 -X github.com/operator-framework/operator-sdk/internal/version.KubernetesVersion=v1.18.8 "  -o dist/operator-sdk-v1.1.0-7-gd7e63051-aarch64-linux-gnu ./cmd/$cmdpkg;

Also, if I go to cd release/ dir and run make release it is no working:

 $ make release
{ \
cmdpkg=$(echo operator-sdk--aarch64-linux-gnu | sed -E "s/(operator-sdk|ansible-operator|helm-operator).*/\1/"); \
go build  -o dist/operator-sdk--aarch64-linux-gnu ./cmd/$cmdpkg; \
}
stat /Users/camilamacedo/go/src/github.com/operator-framework/operator-sdk/release/cmd/operator-sdk: directory not found
make: *** [Makefile:89: dist/operator-sdk--aarch64-linux-gnu] Error 1

joelanford · 2020-10-20T21:05:54Z

@camilamacedo86

ensure that has a tag, do not allow uncommitted files.

You need to run release.sh to get that functionality.

Also, if I go to cd release/ dir and run make release it is no working:

@estroz can correct me if I'm wrong, but that is not intended to work. Perhaps there's a way we can detect that someone called make release from the release directory and error out with something informative that says to run make release from the project root?

IMO, I wouldn't hold this PR up for that though.

estroz · 2020-10-20T21:09:31Z

that is not intended to work

Correct. Often, nested Makefiles are not intended to be used standalone, and will error if you try to. release/Makefile should only be called from the project root.

error out with something informative that says to run make release from the project root

Good idea.

camilamacedo86

Hi @joelanford,

You are right @estroz can correct me if I'm wrong, but that is not intended to work . I do not think that the release is part of this one as well. I thought that make release calls that shell. However, I just checked that is not how it has been in the master too.

👍 Thank you for the explanations.

/lgtm for me this one.

camilamacedo86 · 2020-10-20T21:13:33Z

.travis.yml

-      name: Manifest lists
-      script:
-        - make image-push-ansible-multiarch image-push-helm-multiarch image-push-scorecard-test-multiarch image-push-scorecard-test-kuttl-multiarch image-push-sdk-multiarch
+      script: make -f release/Makefile image-push-multiarch


I liked the cleanup in the Travis 👍

openshift-ci-robot · 2020-10-20T21:25:15Z

New changes are detected. LGTM label has been removed.

estroz · 2020-10-20T21:48:50Z

Example travis deploy (failing because the jobs can't push to operator-framework repos).

openshift-ci-robot requested review from jberkhahn and joelanford October 20, 2020 00:11

estroz force-pushed the chore/update-image-builds branch 3 times, most recently from f4e8d6f to 37e61a3 Compare October 20, 2020 00:54

*: move make targets around such that release-related targets are in

9030482

their own Makefile, ex. image builds are in release/Makefile images/: move all Dockerfiles from hack/image to here, update references hack/: remove unnecessary wrapper scripts

estroz force-pushed the chore/update-image-builds branch from 37e61a3 to 9030482 Compare October 20, 2020 01:17

joelanford reviewed Oct 20, 2020

View reviewed changes

jmrodri self-requested a review October 20, 2020 16:22

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2020

jmrodri approved these changes Oct 20, 2020

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 20, 2020

estroz force-pushed the chore/update-image-builds branch from 77bfc66 to 6c72670 Compare October 20, 2020 20:32

camilamacedo86 reviewed Oct 20, 2020

View reviewed changes

joelanford reviewed Oct 20, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

Makefile Show resolved Hide resolved

release/Makefile Outdated Show resolved Hide resolved

camilamacedo86 previously approved these changes Oct 20, 2020

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Oct 20, 2020

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

estroz mentioned this pull request Oct 20, 2020

Automated downstream Operator SDK into the OpenShift ecosystem. openshift/enhancements#500

Merged

camilamacedo86 requested changes Oct 20, 2020

View reviewed changes

camilamacedo86 approved these changes Oct 20, 2020

View reviewed changes

camilamacedo86 reviewed Oct 20, 2020

View reviewed changes

estroz force-pushed the chore/update-image-builds branch from 6c72670 to d4839f3 Compare October 20, 2020 21:25

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

update Makefiles as per suggested, remove image wrapper scripts

5957cd8

estroz force-pushed the chore/update-image-builds branch from d4839f3 to 5957cd8 Compare October 20, 2020 21:39

estroz merged commit 09c3aa1 into operator-framework:master Oct 21, 2020

estroz deleted the chore/update-image-builds branch October 21, 2020 00:05

estroz mentioned this pull request Oct 28, 2020

images/scorecard-test-kuttl: re-add entrypoint script #4126

Merged

2 tasks

*: refactor Dockerfiles and Makefiles #4069

*: refactor Dockerfiles and Makefiles #4069

Uh oh!

Conversation

estroz commented Oct 20, 2020

Uh oh!

tengqm commented Oct 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmrodri commented Oct 20, 2020

Uh oh!

jmrodri commented Oct 20, 2020

Uh oh!

jmrodri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmrodri commented Oct 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

joelanford commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

estroz commented Oct 20, 2020

Uh oh!

camilamacedo86 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 20, 2020

Uh oh!

estroz commented Oct 20, 2020

Uh oh!

Uh oh!

camilamacedo86 left a comment •

edited

Loading

joelanford commented Oct 20, 2020 •

edited

Loading