-
Notifications
You must be signed in to change notification settings - Fork 2
Refactor Makefile and GH workflows
#7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cf79a59 to
ef96513
Compare
ef96513 to
f679bf5
Compare
|
Ignore the api-ref docs gen for now as we have no |
a6f57f3 to
00b1ea3
Compare
- Refactor `Makefile` to a kubebuilder style format - Adapt GH workflows - Add dependabot configuration
00b1ea3 to
8a61a74
Compare
| **Screenshots** | ||
| If applicable, add screenshots to help explain your problem. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time to understand how screenshots would actually be a good way to explain a problem with a k8s operator. I guess this is just the template copied from somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just some dummy template which needs to be adjusted :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I don't have a strong opinion here, Renovate is actually favored in all https://github.com/SAP-cloud-infrastructure / https://github.com/sapcc so let's decide on this in a bigger round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Me neither. We haven't enabled/used renovate in the ironcore-dev org yet. I guess in the POC state of the project this part is not really a show stopper and can be fixed later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to not go with https://goreleaser.com?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to provide some background, why imo GoReleaser would be preffered:
- Greater adoption throughout the go/k8s ecosystem, incl. usage by kubebuilder (see here), who’s Makefile style we are adapting here.
- GoReleaser also appears to have a more active development:
- GoReleaser:
- Last Commit: Yesterday
- Open Issues: 17
- Release Drafter:
- Last Commit: 6 Months ago
- Open Issues: 142
- GoReleaser:
On a side note, the GoReleaser Project is currently actively sponsored by SAP –https://github.com/sponsors/caarlos0#sponsors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be fine to switch to go releaser to generate the release notes. Let me revert that back.
| - name: Verify kind installation | ||
| run: kind version | ||
|
|
||
| - name: Create kind cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What speaks against using the helm/kind-action@v1 action as used previously?
| run: kind version | ||
|
|
||
| - name: Create kind cluster | ||
| run: kind create cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
|
|
||
| .PHONY: docker-build-controller-manager | ||
| docker-build-controller-manager: ## Build controller-manager. | ||
| docker build --target manager -t ${CONTROLLER_IMG} . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing the BININFO* Build Arguments here. Same applies to other tasks that build the docker image.
| # Copy the go source | ||
| COPY cmd/manager/main.go cmd/manager/main.go | ||
| #COPY api/ api/ | ||
| COPY internal/ internal/ | ||
|
|
||
| # Build | ||
| # the GOARCH has not a default value to allow the binary be built according to the host where the command | ||
| # was called. For example, if we call make docker-build in a local env which has the Apple Silicon M1 SO | ||
| # the docker BUILDPLATFORM arg will be linux/arm64 when for Apple x86 it will be linux/amd64. Therefore, | ||
| # by leaving it empty we can ensure that the container and binary shipped on it will have the same platform. | ||
| FROM builder AS manager-builder | ||
| RUN --mount=type=cache,target=/root/.cache/go-build \ | ||
| --mount=type=cache,target=/go/pkg \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why copy the source files instead of bind-mounting them as did previously. Ref: https://www.docker.com/blog/faster-multi-platform-builds-dockerfile-cross-compilation-guide/
| FROM builder AS manager-builder | ||
| RUN --mount=type=cache,target=/root/.cache/go-build \ | ||
| --mount=type=cache,target=/go/pkg \ | ||
| CGO_ENABLED=0 GOOS=${TARGETOS:-linux} GOARCH=${TARGETARCH} go build -a -o manager cmd/manager/main.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are missing -ldflags '-s -w -X github.com/sapcc/go-api-declarations/bininfo.binName=network-operator -X github.com/sapcc/go-api-declarations/bininfo.version=$(BININFO_VERSION) -X github.com/sapcc/go-api-declarations/bininfo.commit=$(BININFO_COMMIT_HASH) -X github.com/sapcc/go-api-declarations/bininfo.buildDate=$(BININFO_BUILD_DATE) $(GO_LDFLAGS)' here.
Also I'm in favor of striping debug symbols from the binary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the helm chart under charts/network-operator which seems to be inline with the standard convention used by many other projects.
I'm aware that kubebuilder decided for dist/chart but I don't think that's a good choice. There also seems to be a related issue in kubernetes-sigs/kubebuilder#4320, which hopefully gets resolved soon (there are already open PRs for it) to make this configurable.
And to cite the official Helm Documentation:
The directory name is the name of the chart (without versioning information).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to have a kustomize first approach here and let the kubebuilder helm plugin do the rest. @SzymonSAP built in the support for the chart generation in e.g. the metal-operator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is only about the directory, where the plugin generates the chart into.
Using kubebuilder helm plugin has also been done previously to generate the helm chart in the charts/ directory. I'm just saying that this is already there and I would prefer to have it in that location. :)
| docker-buildx: ## Build and push docker image for the manager for cross-platform support | ||
| # copy existing Dockerfile and insert --platform=${BUILDPLATFORM} into Dockerfile.cross, and preserve the original Dockerfile | ||
| sed -e '1 s/\(^FROM\)/FROM --platform=\$$\{BUILDPLATFORM\}/; t' -e ' 1,// s//FROM --platform=\$$\{BUILDPLATFORM\}/' Dockerfile > Dockerfile.cross | ||
| - $(CONTAINER_TOOL) buildx create --name network-operator-builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having CONTAINER_TOOL suggests that anything other than docker would actually be possible, while neither podman nor nerdctl have a buildx create/use command. So for me, this doesn't really make sense. I'm fine if we commit on docker, but then I would leave out the $CONTAINER_TOOL variable.
On another note, the sed call to insert --platform=${BUILDPLATFORM} is not needed, since the Dockerfile already contains that.
I assume that's what kubebuilder generates by default?
|
Closing in favor of #33 |
Makefileto a kubebuilder style formatFixes #6