-
Notifications
You must be signed in to change notification settings - Fork 53
Fix k3d-load make target to support configurable cluster names with target-specific defaults and add documentation #293
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
@danielgerlag 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
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.
- Do not set the CLUSTER_NAME value in any of the higher level makefiles that call other make files.
- Only set the CLUSTER_NAME in a Makefile of an actual project, and only set it to "k3s-default" is it is not set and the target is "k3d-load"
I've addressed your feedback in commit 6c5bad2. The changes now follow the pattern you requested: Higher-level Makefiles (main, control-planes/, query-container/, etc.):
Leaf-level Makefiles (actual project makefiles):
This allows users to set |
k3d-load: CLUSTER_NAME=k3s-default | ||
k3d-load: | ||
k3d image import $(IMAGE_PREFIX)/kubernetes-provider:$(DOCKER_TAG_VERSION) -c $(CLUSTER_NAME) | ||
k3d image import $(IMAGE_PREFIX)/kubernetes-provider:$(DOCKER_TAG_VERSION) -c ${CLUSTER_NAME:-k3s-default} |
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.
@copilot please explain what this does exactly?
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 is shell parameter expansion syntax: ${CLUSTER_NAME:-k3s-default}
. It means:
- If the CLUSTER_NAME environment variable is set and not empty, use its value
- If CLUSTER_NAME is unset or empty, use the default value "k3s-default"
This allows the k3d-load target to use a custom cluster name when provided (e.g., CLUSTER_NAME=my-cluster make k3d-load
) while falling back to "k3s-default" when no custom name is specified (e.g., just make k3d-load
).
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.
Does it work on all platforms, eg. bash, powershell, MacOS, Windows, Linux?
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.
@copilot Does it work on all platforms, eg. bash, powershell, MacOS, Windows, Linux?
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.
You raised a good point about cross-platform compatibility. The shell parameter expansion syntax ${CLUSTER_NAME:-k3s-default}
doesn't work reliably across all platforms and shells (especially on Windows with PowerShell or cmd). I've now replaced it with Make's conditional logic using ifeq ($(CLUSTER_NAME),kind)
which is native Make syntax and works consistently across all platforms. The functionality remains exactly the same, but now it's truly cross-platform compatible. Committed in 9fbb49b.
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.
@copilot Rather set the default using Make, eg. "CLUSTER_NAME ?= k3s-default" than using shell syntax, which may break with different platforms.
You're absolutely right about cross-platform compatibility. I've replaced all shell parameter expansion syntax |
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.
@copilot it is kind of ugly having the initial default set to "kind" and then checking against that for the k3d-load target. Can we clean it up and set the initial default based on the target instead?
I've cleaned up the implementation in commit 0d5ba4a. Instead of the "ugly" pattern of setting a global default to "kind" and then checking against it, I've implemented target-specific defaults:
This approach is much cleaner as each target explicitly defines its appropriate default without any confusing global overrides or conditional checks. The behavior remains exactly the same, but the code is now more maintainable and clear about what default each target uses. |
k3d-load: CLUSTER_NAME=k3s-default | ||
k3d-load: | ||
k3d image import $(IMAGE_PREFIX)/kubernetes-provider:$(DOCKER_TAG_VERSION) -c $(CLUSTER_NAME) | ||
k3d image import $(IMAGE_PREFIX)/kubernetes-provider:$(DOCKER_TAG_VERSION) -c $(if $(CLUSTER_NAME),$(CLUSTER_NAME),k3s-default) |
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.
@copilot is this compatible with all platforms and shell environements? eg. Windows with powershell, bash on MacOS or Linux?
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.
Yes, this syntax $(if $(CLUSTER_NAME),$(CLUSTER_NAME),k3s-default)
is fully cross-platform compatible. This is native Make conditional syntax that's evaluated by Make itself before the command is passed to the shell, so it works regardless of the underlying shell environment (bash, PowerShell, cmd, zsh) or operating system (Windows, macOS, Linux). Unlike shell parameter expansion ${VAR:-default}
which is shell-specific, the Make $(if ...)
function is part of Make's built-in functionality and provides consistent behavior across all platforms where Make is available.
@copilot in our contributing guide (docs/contributing/contributing-code/contributing-code-building/README.md), we currently have instructions on how to use the |
I've added k3d-load examples to the contributing guide at docs/contributing/contributing-code/contributing-code-building/README.md. The documentation now includes both basic usage ( |
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.
LGTM
Co-authored-by: danielgerlag <[email protected]>
Co-authored-by: danielgerlag <[email protected]>
…ss-platform compatibility Co-authored-by: danielgerlag <[email protected]>
…of global defaults Co-authored-by: danielgerlag <[email protected]>
Co-authored-by: ruokun-niu <[email protected]>
b27ba17
to
754d7f9
Compare
The
k3d-load
make target was using a hard-coded cluster name ofk3s-default
, making it impossible to use clusters with different names. Additionally, the original implementation used global defaults with conditional overrides, creating confusing code patterns.Problem
The original implementation had several issues:
CLUSTER_NAME ?= kind
then checkingifeq ($(CLUSTER_NAME),kind)
to override itk3d-load
usage in the contributing guideSolution
This PR implements a clean solution using target-specific defaults and adds comprehensive documentation:
Higher-level Makefiles: Continue to delegate without passing explicit parameters
Leaf-level Makefiles: Use Make's native conditional syntax for target-specific defaults:
Documentation: Added
k3d-load
examples to the contributing guide showing both basic usage and custom cluster name configuration.Usage
Benefits
Fixes #278.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.