Refactor Makefile, GH workflows and other project housekeeping tasks #33

afritzler · 2025-08-20T11:11:10Z

Proposed changes

Refactor Makefile, GH workflows and other project housekeeping tasks.

.github/workflows/kustomize-validation.yml

.github/workflows/publish-chart.yaml

.github/workflows/publish-docker.yml

.github/workflows/test-chart.yml

.github/workflows/test-e2e.yml

Makefile

felix-kaestner · 2025-08-20T14:11:32Z

.github/workflows/checks.yaml

Things we had previously, which I'd prefer to keep:

Spelling Checks

License Checks

Vulnerability Checks

.github/workflows/stale.yaml

felix-kaestner · 2025-08-20T14:12:10Z

dist/chart/.helmignore

As already said in #7 (comment), we already have this chart in https://github.com/ironcore-dev/network-operator/tree/main/charts/network-operator:

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

How is this chart being created? Can the helm plugin be configured to use a different folder?

It's being created with the helm plugin, but then simply moved to another directory, see

network-operator/Makefile

Line 127 in be24d60

@rm -rf charts/network-operator && mv dist/chart charts/network-operator && rm -rf dist

However, there is an open issue on kubebuilder to make this configurable (kubernetes-sigs/kubebuilder#4320) which would simplify this step for us in the future.

As this is already a custom target not included into the kubebuilder scaffold, we can pretty much do what we want here. Also, as this is a complete replacement every time (because the helm plugin just overwrites everything) this step either way always requires some manual efforts.

felix-kaestner · 2025-08-20T14:12:35Z

api/v1alpha1/doc.go

This is already defined in

network-operator/api/v1alpha1/groupversion_info.go

Lines 4 to 8 in 76cc0c8

// Package v1alpha1 contains API Schema definitions for the networking.cloud.sap v1alpha1 API group.

// +kubebuilder:validation:Required

// +kubebuilder:object:generate=true

// +groupName=networking.cloud.sap

package v1alpha1

. I'm fine to move it to a dedicated doc.go, but then we should remove it in the other file.

This file is needed by the CRDs to api reference doc generator.

As said, we can add it. But then we should remove those lines from ./api/v1alpha1/groupversion_info.go#L4-L8 and make sure we keep the following to set the default for the package:

+kubebuilder:validation:Required

api/v1alpha1/doc.go

.golangci.yml

felix-kaestner · 2025-08-22T14:11:01Z

.golangci.yml

+    - copyloopvar
+    - dupl
+    - errcheck
+    - ginkgolinter
+    - goconst
+    - gocyclo
+    - govet
+    - ineffassign
+    - lll
+    - misspell
+    - nakedret
+    - prealloc
+    - revive
+    - staticcheck
+    - unconvert
+    - unparam
+    - unused


This uses way less linters as used previously. Do we really want to reduce the linting in this way?

Added back all the linters. However I don't think we will need things like sqlclosecheck.

.golangci.yml

Makefile

cmd/main.go

felix-kaestner · 2025-08-22T14:26:36Z

test/e2e/e2e_suite_test.go

Please revert the changes done here (except renaming of the make targets used), these were some intentional changes made (and also fixed a lot of linter issues).

felix-kaestner · 2025-09-03T07:40:17Z

.github/workflows/check-codegen.yml

We previously run a check in the Checks workflow to check for license headers. Now that we removed this workflow, can we add this step to this worfklow? In particular it should run the make check-license target we have defined.

felix-kaestner · 2025-09-03T07:41:21Z

.github/workflows/checks.yaml

We were previously running go-licence-detector here. If we remove this now, then we should also remove the .license-scan-rules.json and .license-scan-overrides.jsonl configuration files (or keep it and keep the config).

felix-kaestner · 2025-09-03T07:42:32Z

.github/workflows/lint.yml

+      - name: Run linter
+        run: |
+          make golangci-lint
+          cp ./bin/golangci-lint /usr/local/bin/


nit: we otherwise use mv <binary> /usr/local/bin, e.g. in the kustomize-validation workflow. Ideally this would be consistent.

felix-kaestner · 2025-09-03T07:45:03Z

.github/workflows/publish-docker.yml

+          target: ${{ matrix.image.target }}
+          build-args: |
+            VERSION=${{ steps.meta.outputs.version }}
+            GIT_COMMIT=${{ steps.vars.sha_short }}


Suggested change

GIT_COMMIT=${{ steps.vars.sha_short }}

GIT_COMMIT=$(git rev-parse --short HEAD)

nit: if we can run a shell command here as done below with the date command I would also run this command here inline instead of creating a new step.

Did you test, that for the BUILD_DATE arg you are able to invoke a shell command in this scope?

felix-kaestner · 2025-09-03T07:49:07Z

Dockerfile

+# Copy the Go Modules manifests
+COPY go.mod go.mod
+COPY go.sum go.sum
+# cache deps before building and copying source so that we don't need to re-download as much
+# and so that source changes don't invalidate our downloaded layer
+RUN go mod download


As a suggestion, could we at least do:

RUN --mount=type=cache,target=/go/pkg/mod \ go mod download -x

Instead of the go mod download, so that we have the mod cache to speed up the build process?

felix-kaestner · 2025-09-03T08:20:23Z

test/utils/util.go

+		_, _ = fmt.Fprintf(GinkgoWriter, "chdir dir: %q\n", err)
 	}

+	cmd.Env = append(os.Environ(), "GO111MODULE=on")


Suggested change

cmd.Env = append(os.Environ(), "GO111MODULE=on")

not needed.

felix-kaestner · 2025-09-03T08:22:22Z

test/utils/util.go

+	prometheusOperatorVersion = "v0.77.1"
+	prometheusOperatorURL     = "https://github.com/prometheus-operator/prometheus-operator/" +
+		"releases/download/%s/bundle.yaml"


As you removed the prometheus installation in test/e2e/e2e_suite_test.go. I would either also remove everything regarding that (incl. the InstallPrometheusOperator and UninstallPrometheusOperator funcs that are now unused) or keep the prometheus operator installation as previously.

felix-kaestner · 2025-09-03T08:23:33Z

test/e2e/util.go

@@ -153,31 +171,8 @@ func LoadImageToKindClusterWithName(name string) error {
 	if v, ok := os.LookupEnv("KIND_CLUSTER"); ok {
 		cluster = v
 	}
-	// See: https://kind.sigs.k8s.io/docs/user/rootless/#creating-a-kind-cluster-with-rootless-nerdctl


Please keep this part from the previous implementation, as it allows to use a different container tool than docker (something that kubebuilder pretents but actually does not support).

felix-kaestner · 2025-09-03T08:25:19Z

test/utils/util.go

+	// false positive
+	// nolint:gosec


Suggested change

// false positive

// nolint:gosec

I don't get an gosec linter error here. Same for the end of the func.

felix-kaestner · 2025-09-03T08:26:27Z

test/e2e/util.go

 	if idx < 0 {
-		if strings.Contains(string(content), target[len(prefix):]) {


The previous enhancement of detecting an already commented file is superior. Also, this func is now unused (as we don't install the prometheus operator anymore), so I would remove this func (we shouldn't include dead code in the repo).

github-actions · 2025-11-29T02:14:41Z

This PR is stale because it has been open for 45 days with no activity.

afritzler marked this pull request as ready for review August 20, 2025 11:11

afritzler requested a review from a team as a code owner August 20, 2025 11:11

afritzler force-pushed the enh/makefile branch from 97ccccb to be4e7fb Compare August 20, 2025 11:11

afritzler added the enhancement New feature or request label Aug 20, 2025

afritzler force-pushed the enh/makefile branch 3 times, most recently from bf206f5 to 6b244ce Compare August 20, 2025 11:27

afritzler added ok-to-image ok-to-charts labels Aug 20, 2025

afritzler force-pushed the enh/makefile branch from 6b244ce to 6ddbfd1 Compare August 20, 2025 11:41

afritzler mentioned this pull request Aug 20, 2025

Refactor Makefile and GH workflows #7

Closed

afritzler force-pushed the enh/makefile branch 5 times, most recently from 9890a4b to 6b49a31 Compare August 20, 2025 14:08

felix-kaestner reviewed Aug 20, 2025

View reviewed changes

afritzler force-pushed the enh/makefile branch from 6b49a31 to 4e8fc5b Compare August 20, 2025 14:33

afritzler added the do-not-merge label Aug 20, 2025

afritzler force-pushed the enh/makefile branch from 4e8fc5b to bac45ed Compare August 20, 2025 14:39

felix-kaestner reviewed Aug 20, 2025

View reviewed changes

api/v1alpha1/doc.go Outdated Show resolved Hide resolved

afritzler force-pushed the enh/makefile branch from bac45ed to cbbfd82 Compare August 20, 2025 15:12

hardikdr added the area/metal-automation Automation processes within the Metal project. label Aug 21, 2025

hardikdr added this to Roadmap Aug 21, 2025

afritzler force-pushed the enh/makefile branch 3 times, most recently from dd4f465 to 7a445ba Compare August 21, 2025 08:04

afritzler removed ok-to-image ok-to-charts labels Aug 21, 2025

afritzler force-pushed the enh/makefile branch from 7a445ba to 83dbfb3 Compare August 21, 2025 11:00

afritzler force-pushed the enh/makefile branch 5 times, most recently from 15d85b4 to 49e3275 Compare August 21, 2025 14:58

afritzler removed the do-not-merge label Aug 22, 2025

afritzler force-pushed the enh/makefile branch 3 times, most recently from dd55988 to c2feeef Compare August 22, 2025 09:41

felix-kaestner requested changes Aug 22, 2025

View reviewed changes

afritzler force-pushed the enh/makefile branch from 3bf87e9 to 3556f30 Compare August 27, 2025 10:04

afritzler added the ok-to-charts label Aug 27, 2025

felix-kaestner reviewed Sep 3, 2025

View reviewed changes

afritzler added the ok-to-image label Sep 9, 2025

afritzler force-pushed the enh/makefile branch 2 times, most recently from a6f74bb to f2b63de Compare September 9, 2025 15:41

afritzler added 4 commits October 7, 2025 14:32

Refactor Makefile, GH workflows and other project housekeeping tasks

756336e

PR review

09c3aef

Add version subcommand

0be428d

Fix linting issues

71d8f25

afritzler force-pushed the enh/makefile branch 5 times, most recently from 5ab5751 to 5b75547 Compare October 7, 2025 13:02

Regenerate code

559abfa

afritzler force-pushed the enh/makefile branch from 5b75547 to 559abfa Compare October 7, 2025 13:08

github-actions bot added the stale label Nov 29, 2025

felix-kaestner removed the stale label Nov 29, 2025

	// Package v1alpha1 contains API Schema definitions for the networking.cloud.sap v1alpha1 API group.
	// +kubebuilder:validation:Required
	// +kubebuilder:object:generate=true
	// +groupName=networking.cloud.sap
	package v1alpha1

	GIT_COMMIT=${{ steps.vars.sha_short }}
	GIT_COMMIT=$(git rev-parse --short HEAD)

		if idx < 0 {
		if strings.Contains(string(content), target[len(prefix):]) {

Refactor Makefile, GH workflows and other project housekeeping tasks #33

Are you sure you want to change the base?

Refactor Makefile, GH workflows and other project housekeeping tasks #33

Uh oh!

Conversation

afritzler commented Aug 20, 2025

Proposed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felix-kaestner Aug 22, 2025 • 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!

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!

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!

github-actions bot commented Nov 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

felix-kaestner Aug 22, 2025 •

edited

Loading