Operator Controller Release #133

dtfranz · 2023-02-23T00:38:54Z

Adds a release github action and implements goreleaser, borrowing heavily from rukpak but stripped down a bit.

See my fork here for an example release output: https://github.com/dtfranz/operator-controller/releases/tag/v0.0.2

Some of the additional build targets found in rukpak have been stripped out to keep this smaller.

Addresses #132 and #109

perdasilva · 2023-02-27T12:46:43Z

.dockerignore

@@ -1,4 +1,3 @@
 # More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file
-# Ignore build and test binaries.
-bin/
+# Ignore test binaries.


it seems we are using bin as the context for building the image - do we still need the .dockerignore?

perdasilva · 2023-02-27T12:47:59Z

Makefile

@@ -132,6 +132,24 @@ docker-buildx: test ## Build and push docker image for the manager for cross-pla
 	- docker buildx rm project-v3-builder
 	rm Dockerfile.cross

+###########


can we get some make help docs around release - when/how it should be used?

perdasilva · 2023-02-27T12:49:46Z

internal/version/version.go

+package version
+
+// GitCommit indicates which commit the rukpak binaries were built from
+var GitCommit string


does the value get injected at build time? is it worth adding a comment?

+1.

Also, I don't see this used anywhere else in the Go binary. Maybe we should take this out of this PR and create a separate issue to remind us to get version info into the binary and have a way to print it out.

IIRC the go compiler as of 1.18 or 1.19 now injects this git info automatically. We might not need to do it ourselves anymore.

perdasilva

lgtm - I just had a couple of small questions/suggestions. Nothing blocking.

joelanford

Looks pretty good overall! The Rukpak setup mostly works, but I think it was somewhat rushed and probably is in need of some fine tuning.

I left some comments along those lines, but there's probably more to do as well. Regardless, I think further improvements could be done in one or more follow-ons.

joelanford · 2023-02-27T13:07:05Z

Dockerfile

-FROM golang:1.19 as builder
-ARG TARGETOS
-ARG TARGETARCH
+FROM gcr.io/distroless/static:debug-nonroot


I don't think we need the debug variant for operator-controller do we?

Suggested change

FROM gcr.io/distroless/static:debug-nonroot

FROM gcr.io/distroless/static:nonroot

joelanford · 2023-02-27T13:09:46Z

Dockerfile


-ENTRYPOINT ["/manager"]
+COPY manager manager
+


Do we need to explicitly specify the user? Honestly not sure. I think my suggestion would be to specify it explicitly rather than inherit it from the base image.

We should also specify the entrypoint since this is the only binary we'll be using in this image.

Suggested change

USER 65532:65532

ENTRYPOINT ["/manager"]

joelanford · 2023-02-27T13:12:35Z

Makefile

@@ -95,10 +95,10 @@ kind-cluster-cleanup: kind ## Delete the kind cluster

 .PHONY: build
 build: manifests generate fmt vet ## Build manager binary.
-	go build -o bin/manager main.go
+	CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o bin/manager main.go


I discovered goreleaser build recently. WDYT about using that instead? The big benefit is that we essentially get local builds that use the exact same configuration as the release builds (same env, same ldflags, etc.)

joelanford · 2023-02-27T13:15:44Z

Makefile

@@ -109,7 +109,7 @@ wait:
 # More info: https://docs.docker.com/develop/develop-images/build_enhancements/
 .PHONY: docker-build
 docker-build: generate ## Build docker image with the operator-controller.
-	docker build -t ${IMG} .
+	docker build -t ${IMG} -f Dockerfile ./bin/


This seems like it'll fail because the Dockerfile expects the binary to be pre-built by goreleaser. The previous Dockerfile included the binary build as a pre-step in a multi-stage build.

I think we need to add build to the list of pre-reqs?

Also, the binary built in this case needs to be GOOS=linux, but in the non-docker build case should just take the default settings of the environment. That way, mac and linux machines can both build local binaries and container image binaries correctly.

I've updated the build target to use goreleaser build, and I believe that's also resolved your concern here. By default, when running make build, goreleaser will set GOARCH and GOOS to the user's setup. This won't apply though when building for release.

For docker-build I think we need to explicitly set GOOS=linux, right? If we don't won't a macOS user end up with a docker container that contains a darwin binary?

TBH, building within the container generally improves compatibility within the container and improves build reproducibility.

GoReleaser doesn't support that, so if we want binary builds happening inside docker, we can't use goreleaser I don't think.

I don't love this restriction, but my main interests are:

we have exactly one way to build the the binaries snd containers

the binaries we build ARE the binaries that end up in the container.

GoReleaser can be made to ensure reproducibility (except maybe Go compiler version) so I think we end up at the same place. It just takes a bit more care to get there.

joelanford · 2023-02-27T13:16:48Z

Makefile

 .PHONY: docker-push
 docker-push: ## Push docker image with the manager.


WDYT about removing docker-push? We don't use it anywhere else, and it isn't part of the normal dev flow.

joelanford · 2023-02-27T13:19:42Z

Makefile

+substitute:
+	envsubst < .goreleaser.template.yml > .goreleaser.yml


Is this used anywhere other than release? If not, I'd propose to consolidate this into the release recipe.

Having a top-level target called substitute seems like it could be both too vague and potentially confusing for contributors wondering if/when they should use it.

joelanford · 2023-02-27T13:21:26Z

internal/version/version.go

+func String() string {
+	return GitCommit
+}


I don't see this used anywhere. Remove it? Seems like anything that needs to know the git commit can just use version.GitCommit directly.

joelanford · 2023-02-27T13:36:34Z

Makefile

+
+quickstart: VERSION ?= $(shell git describe --abbrev=0 --tags)
+quickstart: generate ## Generate the installation release manifests
+	kubectl kustomize config/default | sed "s/:latest/:$(VERSION)/g" > operator-controller.yaml


Kustomize can do image ref substitution directly. We may want to consider whether that is a viable alternative. If we aren't very careful, this approach could silently replace other image refs in our manifests if they happen to use :latest.

Really anything referencing latest in our manifests would be a problem, so maybe in practice this would be fine. But I'd rather not compound that problem if it does somehow happen.

Yeah I can definitely see that being a problem. Let me fix this up to make sure we're just using the substitutions provided by kustomize.

OK, with the latest push I have this being done through kustomize using the VERSION env set during release. Let me know what you think :)

joelanford · 2023-02-27T15:41:46Z

.goreleaser.template.yml

+  use: github-native
+  skip: $DISABLE_RELEASE_PIPELINE
+release:
+  disable: $DISABLE_RELEASE_PIPELINE


I'm trying to remember why we do this convoluted envsubst thing rather than use the --snapshot flag when invoking goreleaser.

Ah I remember: we want to build/push images/manifests -- but NOT release -- for builds of the main branch.

It seems like this problem is half solved. It's possible to template release.disable, but still not possible to template changelog.skip.

I opened an issue in goreleaser to see if we can make changelog.skip templatable as well: goreleaser/goreleaser#3828

Sweet. The maintainer already merged a PR that fixes this. Once there's a release, we can directly reference {{ .Env.DISABLE_RELEASE_PIPELINE }} as a template value in the actual goreleaser config file. No need for the envsubst shenanigans after that.

joelanford · 2023-02-27T15:47:19Z

.goreleaser.template.yml

+    ```bash
+    kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/{{ .Env.CERT_MGR_VERSION }}/cert-manager.yaml
+    kubectl wait --for=condition=Available --namespace=cert-manager deployment/cert-manager-webhook --timeout=60s
+    kubectl apply -f https://github.com/operator-framework/rukpak/releases/latest/download/rukpak.yaml
+    kubectl wait --for=condition=Available --namespace=rukpak-system deployment/core --timeout=60s
+    kubectl wait --for=condition=Available --namespace=rukpak-system deployment/helm-provisioner --timeout=60s
+    kubectl wait --for=condition=Available --namespace=rukpak-system deployment/rukpak-webhooks --timeout=60s
+    kubectl apply -f https://github.com/operator-framework/operator-controller/releases/download/{{ .Tag }}/operator-controller.yaml
+    kubectl wait --for=condition=Available --namespace=operator-controller-system deployment/operator-controller --timeout=60s
+    ```


This is getting somewhat long. I suppose this is basically our "distribution", right? I wonder if we should think about consolidating this into a script and then having the header be a one-liner that shows how to curl | sh the script? (could be a follow-on)

Yeah, this is basically the distribution method at the moment. We have an upcoming issue though that should accomplish what you're asking for here: #110

joelanford · 2023-03-01T18:24:38Z

Dockerfile

-FROM golang:1.19 as builder
-ARG TARGETOS
-ARG TARGETARCH
+FROM gcr.io/distroless/static:nonroot


Can we add a comment to this file that says it's intended to be used with make docker-build or make release?

joelanford · 2023-03-01T18:45:54Z

Makefile

+.PHONY: goreleaser
+goreleaser: $(GORELEASER) ## Builds a local copy of goreleaser
+$(GORELEASER): $(LOCALBIN)
+	test -s $(LOCALBIN)/goreleaser || GOBIN=$(LOCALBIN) go install github.com/goreleaser/goreleaser@v1.11.2


For follow-up, but I'm noticing right now -- we should move LOCALBIN somewhere else because the docker build copies all these build tool binaries into the build context unnecessarily, which makes builds take longer.

Rukpak puts these in ./tools/bin and reserves ./bin for binaries built from the repo itself.

Great point, I'll create a follow-up issue to take care of both these items.

dtfranz · 2023-03-01T22:33:22Z

@joelanford @perdasilva I believe I've addressed all your comments, please take another look when you have a moment.

joelanford · 2023-03-02T02:33:05Z

e2e is still failing for me on my Mac. To fix it for me, I added this line, directly above the declaration of the docker-build target:

docker-build: export GOOS=linux

tmshort · 2023-03-02T15:38:10Z

.dockerignore

-# More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file
-# Ignore build and test binaries.
-bin/
-testbin/


Why delete?

Good point, I'll have this ignore all the tools binaries to speed up the build. We can remove it once the tools live in a different folder since there shouldn't be anything left to ignore for docker.

dtfranz · 2023-03-02T18:50:40Z

e2e is still failing for me on my Mac. To fix it for me, I added this line, directly above the declaration of the docker-build target:
docker-build: export GOOS=linux

The goreleaser config file wasn't being generated which is what was probably causing all of these environment based failures. I've fixed that issue now, so please pull and test again when you can :)

joelanford · 2023-03-03T01:44:49Z

Still failing for me on macOS (arm64, but I don't think that matters). After I run make e2e, I see this:

$ file ./bin/manager
./bin/manager: Mach-O 64-bit executable arm64

So I'm still getting a darwin binary. Same fix from before still fixes it for me.

dtfranz · 2023-03-03T06:45:53Z

Still failing for me on macOS (arm64, but I don't think that matters). After I run make e2e, I see this:
$ file ./bin/manager
./bin/manager: Mach-O 64-bit executable arm64
So I'm still getting a darwin binary. Same fix from before still fixes it for me.

Strange, it's not clear to me why that fix is necessary since the goos is set to linux in the goreleaser template for both the binary and docker build. I've updated the branch again with your fix in the meantime.

joelanford · 2023-03-03T14:41:57Z

Strange, it's not clear to me why that fix is necessary since the goos is set to linux in the goreleaser template for both the binary and docker build.

I think the goreleaser build --single-target always uses GOOS and GOARCH, regardless of what's in the goreleaser file.

As far as the docker build, the config in the goreleaser file is only used during a release. The docker-build Makefile target doesn't use the goreleaser config at all.

This docker build stuff could maybe be improved some as well (maybe the docker-build target should also invoke goreleaser somehow?) But that seems better for a follow-up.

joelanford · 2023-03-03T14:56:03Z

.goreleaser.template.yml

+    goos:
+    - linux


Not sure if GH ate my comment or if I managed not to submit it.

This seems to build only a single binary for linux/$GOARCH. But we need to build binaries for the following:

linux/amd64

linux/arm64

linux/ppc64le

linux/s390x

We then need to propagate these to separate individual docker images, and then build the manifest list that combines all of them.

All this is probably fine to do in a follow-up, but might be good to do that before we tag this repo the first time.

joelanford

/lgtm

Nice work! A few more things are better left for smaller follow-ups:

Multiarch binaries and containers
Remove the envsubst and goreleaser template file when there's a new release of goreleaser
Move tools binaries to separate directory.
Investigate calling goreleaser as part of the docker-build target.

Anything I missed?

Adds a release github action and implements goreleaser, borrowing heavily from rukpak but stripped down a bit. Signed-off-by: dtfranz <dfranz@redhat.com>

dtfranz · 2023-03-07T00:18:48Z

/lgtm

Nice work! A few more things are better left for smaller follow-ups:

1. Multiarch binaries and containers

2. Remove the `envsubst` and goreleaser template file when there's a new release of goreleaser

3. Move tools binaries to separate directory.

4. Investigate calling goreleaser as part of the `docker-build` target.

Anything I missed?

Thanks @joelanford! Yes, I think that about sums it up.
I did some testing with all the latest changes and found some issues when running the releaser in my fork. goreleaser doesn't play well with using kustomize to modify the manifest files, so after thinking about it probably for too long and not seeing a clean and easy solution I went with a compromise of keeping rukpak's sed replace for the image tag but used :devel instead of :latest so we'll be far less likely to hit a tag we didn't intend. If that's a dealbreaker let me know, otherwise I'll merge once I get another LGTM after the latest changes 😃

joelanford · 2023-03-07T00:38:18Z

Totally reasonable. I think that's another small cleanup thing we can do once this heavy lift has merged.

/lgtm

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 23, 2023

dtfranz force-pushed the operator-controller-release branch from e462cea to e812ab5 Compare February 23, 2023 17:56

dtfranz changed the title ~~WIP: Operator Controller Release~~ Operator Controller Release Feb 23, 2023

dtfranz marked this pull request as ready for review February 23, 2023 18:23

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 23, 2023

dtfranz force-pushed the operator-controller-release branch 2 times, most recently from 46447e9 to af6453c Compare February 23, 2023 19:02

This was referenced Feb 23, 2023

Create installation yaml for operator-controller #109

Closed

Create release pipeline for operator-controller #132

Closed

perdasilva reviewed Feb 27, 2023

View reviewed changes

perdasilva approved these changes Feb 27, 2023

View reviewed changes

joelanford reviewed Feb 27, 2023

View reviewed changes

dtfranz force-pushed the operator-controller-release branch 4 times, most recently from b338664 to 3eba91d Compare March 1, 2023 08:34

joelanford reviewed Mar 1, 2023

View reviewed changes

dtfranz force-pushed the operator-controller-release branch 3 times, most recently from 0f860a5 to d57ed68 Compare March 1, 2023 22:23

tmshort reviewed Mar 2, 2023

View reviewed changes

dtfranz force-pushed the operator-controller-release branch 2 times, most recently from 963d8a6 to 1bf6eef Compare March 2, 2023 17:56

dtfranz force-pushed the operator-controller-release branch from 1bf6eef to 034bac8 Compare March 2, 2023 19:30

dtfranz force-pushed the operator-controller-release branch from 034bac8 to 0ec29f4 Compare March 3, 2023 06:44

joelanford reviewed Mar 3, 2023

View reviewed changes

joelanford approved these changes Mar 3, 2023

View reviewed changes

openshift-ci bot assigned joelanford Mar 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2023

perdasilva added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2023

Operator Controller Release

26b9335

Adds a release github action and implements goreleaser, borrowing heavily from rukpak but stripped down a bit. Signed-off-by: dtfranz <dfranz@redhat.com>

dtfranz force-pushed the operator-controller-release branch from 0ec29f4 to 26b9335 Compare March 7, 2023 00:14

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2023

dtfranz merged commit eb0312c into operator-framework:main Mar 7, 2023

	FROM gcr.io/distroless/static:debug-nonroot
	FROM gcr.io/distroless/static:nonroot

		.PHONY: docker-push
		docker-push: ## Push docker image with the manager.

		substitute:
		envsubst < .goreleaser.template.yml > .goreleaser.yml

Operator Controller Release #133

Operator Controller Release #133

Conversation

dtfranz commented Feb 23, 2023 • edited Loading

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

perdasilva left a comment

Choose a reason for hiding this comment

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

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

joelanford Mar 3, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtfranz commented Mar 1, 2023

joelanford commented Mar 2, 2023

Choose a reason for hiding this comment

dtfranz Mar 2, 2023 • edited Loading

Choose a reason for hiding this comment

dtfranz commented Mar 2, 2023 • edited Loading

joelanford commented Mar 3, 2023

dtfranz commented Mar 3, 2023

joelanford commented Mar 3, 2023 • edited Loading

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

dtfranz commented Mar 7, 2023

joelanford commented Mar 7, 2023

dtfranz commented Feb 23, 2023 •

edited

Loading

joelanford Mar 3, 2023 •

edited

Loading

dtfranz Mar 2, 2023 •

edited

Loading

dtfranz commented Mar 2, 2023 •

edited

Loading

joelanford commented Mar 3, 2023 •

edited

Loading