fix(makefile): Fixes to make test-e2e idempotent and dependency free - #251
Merged
Conversation
COMMON_SRC_ROOT, DEB_SRC_ROOT and RPM_SRC_ROOT were plain directories, populated by copying files into them. Make considers a directory up to date the moment it exists. This means a failed prior run with partial artifacts will continue to report the directory as up to date. Hit this issue when I had a stale debsrc/ from a previous test run which caused the following error: ``` Error: Binary file not found at /binary/pf9-byoh-hostagent dpkg: error processing package pf9-byohost-agent (--install): installed pf9-byohost-agent package post-installation script subprocess returned error exit status 1 ``` Also, both DEB_SRC_ROOT and RPM_SRC_ROOT depend on COMMON_SRC_ROOT as an order-only prerequisite, which means when COMMON_SRC_ROOT is rebuilt, the changes never actually propagate to them. To fix this, add a stamp file to each directory – a target with its own mtime that Make can use to verify if the directory's contents is stale compared to the stamp file or not.
By default, test-e2e does not build the agent's deb bundle that's required in the byohctl e2e test. ensureLocalAgentBundleRegistry only checked whether build/pf9-byohost/debsrc/pf9-byohost-agent.deb already exists, so a local e2e test skips it with (but not before taking 2 minutes to setup everything): ``` [SKIPPED] no agent bundle available for byohctl's SetupAgent to install (neither BYOH_AGENT_BUNDLE_URL nor a local `make build-host-agent-deb` output was found; see ensureLocalAgentBundleRegistry ...) ``` This is fixable by running `make build-host-agent-deb` first, but the dependency should be explicit and automatic. To avoid this, add build-host-agent-deb as a dependency for test-e2e. And to ensure that CI does not rebuild the bundle and is instead able to download the artifact that was already created, gate it with SKIP_BUILD. Finally, also bump the e2e Ginkgo run to -vv, because I saw the following in the test output: ``` There were additional failures detected. To view them in detail run ginkgo -vv ```
Comment on lines
+262
to
+268
| ifdef SKIP_BUILD | ||
| BUILD_HOST_AGENT_DEB_PREREQ := | ||
| else | ||
| BUILD_HOST_AGENT_DEB_PREREQ := build-host-agent-deb | ||
| endif | ||
|
|
||
| test-e2e: take-user-input docker-build prepare-byoh-docker-host-image $(GINKGO) cluster-templates-e2e $(BUILD_HOST_AGENT_DEB_PREREQ) ## Run the end-to-end tests |
Collaborator
There was a problem hiding this comment.
Suggested change
| ifdef SKIP_BUILD | |
| BUILD_HOST_AGENT_DEB_PREREQ := | |
| else | |
| BUILD_HOST_AGENT_DEB_PREREQ := build-host-agent-deb | |
| endif | |
| test-e2e: take-user-input docker-build prepare-byoh-docker-host-image $(GINKGO) cluster-templates-e2e $(BUILD_HOST_AGENT_DEB_PREREQ) ## Run the end-to-end tests | |
| test-e2e: test-e2e-inner build-host-agent-deb | |
| test-e2e-inner: take-user-input docker-build prepare-byoh-docker-host-image $(GINKGO) cluster-templates-e2e $(BUILD_HOST_AGENT_DEB_PREREQ) ## Run the end-to-end tests |
Collaborator
Author
There was a problem hiding this comment.
@sebastian-pf9 I don't get this. This will always run build-host-agent-deb when test-e2e is called. And in CI that means we will build the agent bundle again even though the artifact was available already.
Collaborator
There was a problem hiding this comment.
have the gihub-ci call test-e2e-inner instead?
Collaborator
Author
There was a problem hiding this comment.
I'd prefer to use the same target from both CI and local dev. Otherwise we will end up with issues that are going to be seen from CI but not from local dev and vice versa. We already do the SKIP_BUILD approach for docker-build target for example.
Collaborator
There was a problem hiding this comment.
no ifdef SKIP_BUILD. please! This is so ugly. don't do it.
Collaborator
Author
There was a problem hiding this comment.
Ack. What's the recommended alternative with Makefile if we want to:
- Use the same external target from both CI and local dev
- Conditionally skip specific parts of the target
sebastian-pf9
approved these changes
Sep 1, 2026
sebastian-pf9
left a comment
sebastian-pf9
left a comment
Collaborator
There was a problem hiding this comment.
approve to unblock - no ifdef - please
Collaborator
Author
|
Merging what we have. I think the build system is already quite complicated and might require a thin sh script as a layer to detect CI vs local dev envs. Will revisit. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and why
Two fixes to ensure running test-e2e on a local setup does not fail because of stale or missing artifacts. See each commit for details.
Fixes KAAP-2375.
Assisted by
Sonnet-5 Medium.