Skip to content

Conversation

@afritzler
Copy link
Member

@afritzler afritzler commented Jul 11, 2025

  • Refactor Makefile to a kubebuilder style format
  • Adapt GH workflows
  • Add dependabot configuration

Fixes #6

@afritzler afritzler requested review from a team as code owners July 11, 2025 08:27
@afritzler afritzler force-pushed the chore/project-setup branch from cf79a59 to ef96513 Compare July 11, 2025 08:28
@afritzler afritzler force-pushed the chore/project-setup branch from ef96513 to f679bf5 Compare July 11, 2025 08:31
@afritzler
Copy link
Member Author

Ignore the api-ref docs gen for now as we have no api package yet.

@afritzler afritzler force-pushed the chore/project-setup branch 3 times, most recently from a6f57f3 to 00b1ea3 Compare July 11, 2025 09:13
- Refactor `Makefile` to a kubebuilder style format
- Adapt GH workflows
- Add dependabot configuration
@afritzler afritzler force-pushed the chore/project-setup branch from 00b1ea3 to 8a61a74 Compare July 11, 2025 09:17
Comment on lines +18 to +20
**Screenshots**
If applicable, add screenshots to help explain your problem.

Copy link
Contributor

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?

Copy link
Member Author

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 :)

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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?

Copy link
Contributor

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:

  1. Greater adoption throughout the go/k8s ecosystem, incl. usage by kubebuilder (see here), who’s Makefile style we are adapting here.
  2. 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

On a side note, the GoReleaser Project is currently actively sponsored by SAP –https://github.com/sponsors/caarlos0#sponsors.

Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

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} .
Copy link
Contributor

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.

Comment on lines +25 to +37
# 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 \
Copy link
Contributor

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
Copy link
Contributor

@felix-kaestner felix-kaestner Jul 11, 2025

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.

Copy link
Contributor

@felix-kaestner felix-kaestner Jul 11, 2025

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).

Copy link
Member Author

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.

Copy link
Contributor

@felix-kaestner felix-kaestner Jul 16, 2025

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
Copy link
Contributor

@felix-kaestner felix-kaestner Jul 11, 2025

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?

@hardikdr hardikdr added this to Roadmap Jul 12, 2025
@hardikdr hardikdr added the area/metal-automation Automation processes within the Metal project. label Jul 15, 2025
@afritzler
Copy link
Member Author

Closing in favor of #33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/metal-automation Automation processes within the Metal project. chore ok-to-image

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Use kubebuilder style Makefile

4 participants