Skip to content

Conversation

@moelsayed
Copy link
Contributor

@moelsayed moelsayed commented Apr 30, 2025

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
This PR changes the artifact paths from ghcr.io/openmcp-project/github.com/openmcp-project/... to ghcr.io/openmcp-project/....
Release note:

Taskfile.yaml Outdated
Comment on lines 16 to 22
excludes:
- test
- test-envtest-dep
- build:bin:val:test
- build-raw
- build:bin:build-raw
# put task names in here which are overwritten in this file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
excludes:
- test
- test-envtest-dep
- build:bin:val:test
- build-raw
- build:bin:build-raw
# put task names in here which are overwritten in this file

Probably not needed, see below.

Taskfile.yaml Outdated
Comment on lines 33 to 93
common: # imported a second time so that overwriting task definitions can call the overwritten task with a 'c:' prefix
taskfile: hack/common/Taskfile_controller.yaml
internal: true
aliases:
- c
vars:
NESTED_MODULES: api
API_DIRS: '{{.ROOT_DIR}}/api/v1beta1/...'
MANIFEST_OUT: '{{.ROOT_DIR}}/api/crds/manifests'
CODE_DIRS: '{{.ROOT_DIR}}/cmd/... {{.ROOT_DIR}}/internal/... {{.ROOT_DIR}}/test/... {{.ROOT_DIR}}/api/v1beta1/...'
COMPONENTS: 'control-plane-operator'
REPO_URL: 'https://github.com/openmcp-project/control-plane-operator'
GENERATE_DOCS_INDEX: "true"
ENVTEST_REQUIRED: "true"
ENVTEST_K8S_VERSION: "1.30.0"

tasks:
test:
desc: " Run all tests."
run: once
aliases:
- build:bin:val:test
vars:
NESTED_MODULES: api
API_DIRS: '{{.ROOT_DIR}}/api/v1beta1/...'
MANIFEST_OUT: '{{.ROOT_DIR}}/api/crds/manifests'
CODE_DIRS: '{{.ROOT_DIR}}/cmd/... {{.ROOT_DIR}}/internal/... {{.ROOT_DIR}}/test/... {{.ROOT_DIR}}/api/v1beta1/...'
COMPONENTS: 'control-plane-operator'
REPO_URL: 'https://github.com/openmcp-project/control-plane-operator'
GENERATE_DOCS_INDEX: "true"
ENVTEST_REQUIRED: "true"
ENVTEST_K8S_VERSION: "1.30.0"
deps:
- test-envtest-dep
cmds:
- 'PROJECT_ROOT="{{.ROOT_DIR2}}" NESTED_MODULES="{{.NESTED_MODULES}}" ENVTEST_K8S_VERSION="{{.ENVTEST_K8S_VERSION}}" {{.TASKFILE_DIR2}}/run-tests.sh {{.CODE_DIRS}}'

test-envtest-dep:
desc: " Install the envtest dependency, if marked as required."
run: once
status:
- '[ "{{.ENVTEST_REQUIRED | default "false"}}" != "true" ]'
cmds:
- task: tools:envtest
vars:
ENVTEST_REQUIRED: "true"
internal: true

build-raw:
desc: " Build the binary. Opposed to the regular build, this one just builds and skips code generation/validation tasks."
summary: |
This task builds the binary for the current operating system and architecture.
To overwrite this, set the 'OS' and 'ARCH' environment variables.
The binary is saved in the 'bin' folder, as $COMPONENT.$OS-$ARCH.
aliases:
- build:bin:build-raw
cmds:
- for:
var: COMPONENTS
as: COMPONENT
cmd: 'CGO_ENABLED=0 GOOS={{.OS}} GOARCH={{.ARCH}} go build -a -o {{.ROOT_DIR2}}/bin/{{.COMPONENT}}.{{.OS}}-{{.ARCH}} {{.ROOT_DIR2}}/cmd/main.go' No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
common: # imported a second time so that overwriting task definitions can call the overwritten task with a 'c:' prefix
taskfile: hack/common/Taskfile_controller.yaml
internal: true
aliases:
- c
vars:
NESTED_MODULES: api
API_DIRS: '{{.ROOT_DIR}}/api/v1beta1/...'
MANIFEST_OUT: '{{.ROOT_DIR}}/api/crds/manifests'
CODE_DIRS: '{{.ROOT_DIR}}/cmd/... {{.ROOT_DIR}}/internal/... {{.ROOT_DIR}}/test/... {{.ROOT_DIR}}/api/v1beta1/...'
COMPONENTS: 'control-plane-operator'
REPO_URL: 'https://github.com/openmcp-project/control-plane-operator'
GENERATE_DOCS_INDEX: "true"
ENVTEST_REQUIRED: "true"
ENVTEST_K8S_VERSION: "1.30.0"
tasks:
test:
desc: " Run all tests."
run: once
aliases:
- build:bin:val:test
vars:
NESTED_MODULES: api
API_DIRS: '{{.ROOT_DIR}}/api/v1beta1/...'
MANIFEST_OUT: '{{.ROOT_DIR}}/api/crds/manifests'
CODE_DIRS: '{{.ROOT_DIR}}/cmd/... {{.ROOT_DIR}}/internal/... {{.ROOT_DIR}}/test/... {{.ROOT_DIR}}/api/v1beta1/...'
COMPONENTS: 'control-plane-operator'
REPO_URL: 'https://github.com/openmcp-project/control-plane-operator'
GENERATE_DOCS_INDEX: "true"
ENVTEST_REQUIRED: "true"
ENVTEST_K8S_VERSION: "1.30.0"
deps:
- test-envtest-dep
cmds:
- 'PROJECT_ROOT="{{.ROOT_DIR2}}" NESTED_MODULES="{{.NESTED_MODULES}}" ENVTEST_K8S_VERSION="{{.ENVTEST_K8S_VERSION}}" {{.TASKFILE_DIR2}}/run-tests.sh {{.CODE_DIRS}}'
test-envtest-dep:
desc: " Install the envtest dependency, if marked as required."
run: once
status:
- '[ "{{.ENVTEST_REQUIRED | default "false"}}" != "true" ]'
cmds:
- task: tools:envtest
vars:
ENVTEST_REQUIRED: "true"
internal: true
build-raw:
desc: " Build the binary. Opposed to the regular build, this one just builds and skips code generation/validation tasks."
summary: |
This task builds the binary for the current operating system and architecture.
To overwrite this, set the 'OS' and 'ARCH' environment variables.
The binary is saved in the 'bin' folder, as $COMPONENT.$OS-$ARCH.
aliases:
- build:bin:build-raw
cmds:
- for:
var: COMPONENTS
as: COMPONENT
cmd: 'CGO_ENABLED=0 GOOS={{.OS}} GOARCH={{.ARCH}} go build -a -o {{.ROOT_DIR2}}/bin/{{.COMPONENT}}.{{.OS}}-{{.ARCH}} {{.ROOT_DIR2}}/cmd/main.go'

I remember that I did this somewhere and you probably copied it from there, but I couldn't find it right now.
When I did this, the envtest stuff was not contained in the library, so it had to be added by these overwrites. By now, it is contained (see https://github.com/openmcp-project/build/blob/main/tasks_val.yaml#L23-L41), which means that simply setting ENVTEST_REQUIRED to "true" should be enough. Then, no overwrites are required, which means that the second include isn't required either (because it is only needed to refer to the original implementation in the overwrites).
This theory needs to be validated by running task test without the marked lines, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is not envtest itself, the problem is that this particular instance expects the ENVTEST_K8S_VERSION environment variable which is not available to the run-test.sh script. running task test is what started all this :D

docs/README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this and disable documentation index generation for now, see remark above.

if len(version) != 0 {
return version, nil
}
// fall back to reading the makefile!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no Makefile anymore, so this fallback needs to be adapted or removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CRDs were previously under config/crd/bases, where they are now not updated anymore. My proposal would be to make the aforementioned path a symlink to api/crds/manifests.

@Diaphteiros
Copy link
Contributor

Diaphteiros commented May 6, 2025

I understand the problem with the KUBEBUILDER_ASSETS variable for the envtest requirement, but I would really like to be able to enable envtest without having to overwrite half of the validation tasks 😞

But I think we will get this to work somehow, with a few hacky modifications to the shared coding.
What about this:
The envtest tool task (the one that downloads the setup-envtest stuff) also runs this setup-envtest use command and dumps the resulting path into a file in the LOCALBIN folder. In the test task, we simply always set KUBEBUILDER_ASSETS=$(cat <aforementioned file> &>/dev/null || echo -n "") or something like this. If envtest is not required, the value of the KUBEBUILDER_ASSETS env var doesn't matter (I hope). If it is required, the file should exist and contain a valid path. We might need to adapt the status check for the envtest tool. Do you think something like this could work? Would be nicer than having to overwrite the tasks whenever envtest is required.

Copy link
Contributor

@Diaphteiros Diaphteiros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants