🌱 fix: update kind-load target to ensure that works aligned with controller-operator #462
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -215,8 +215,7 @@ kind-cluster-cleanup: $(KIND) ## Delete the kind cluster | |
|
|
||
| .PHONY: kind-load | ||
| kind-load: check-cluster $(KIND) ## Load the built images onto the local cluster | ||
| docker save $(IMAGE) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME) | ||
|
There was a problem hiding this comment. Why replace common call to Kind with specific call to a particular container-runtime? There was a problem hiding this comment. @bentito there is currently no support for |
||
|
|
||
| .PHONY: install | ||
| install: check-cluster build-container kind-load deploy wait ## Install local catalogd to an existing cluster | ||
|
|
||
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.
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.
I vaguely remember that we already tried something like this and there were issues with some setups. I think, was either macOS vs Linux setups or Docker vs Podman. Not sure.
What did you mean here? Was the intention to use
kindfromPATH? This change still uses bingo-provided version of kind.And I'm not sure that we want to do that do that: kind and other dependencies are intentionally managed by with so we have reproducibility across environments.
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.
I want to use my own local kind installation instead of relying on a version tied to bingo (e.g.,
/Users/camilam/go/bin/kind-v0.24.0). Contributors should have the flexibility to work with their local environments without unnecessary dependencies.Forcing a specific kind of version is restrictive, and global dependencies are unnecessary and a pain. It messed up with my local env.
/Users/camilam/go/bin/kind-v0.24.0to run any command for kind. It was frustrating to figure out why the image wasn’t loading.This approach works perfectly on macOS (I use a Mac, and the CI uses Linux, so we are 100% safe here) and matches the implementation used in con-oper, where it’s not a problem.
Why treat it differently here? Standardizing this method simplifies workflows and makes the project more contributor-friendly. Let’s avoid unnecessary friction for contributors.
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.
As I said - I vaguely remember us having issues with kind load. But if it is the same approach as in operator-controller - then we should be good because I believe it was sorted in operator-controller. Here is the background I managed to dig out:
This change does not achieve that:
makestill uses bingo versioned kind. Don't hesitate to submit a PR to further address this.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 are right
I will do a follow up to fix the kind and do not use it from BINGO
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.
I really disagree with this actually. If the repository is setup to seamlessly ensure the necessary build/test dependencies are present on the local system, there is no good reason:
The more options and overrides that are made available in the Makefile, the harder it is to trust the results of any particular CI task.
To the extent that we can use tooling like bingo to provide specific build tool versions, we should enforce those build tool versions for all contributors. It improves consistency and eliminates most of the "well it works on my system" problems.
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.
I agree with what @joelanford said above and this is what I meant above when I said that we intentionally manage dependencies like this in the repo with bingo (but I now see that I typed some nonsense there).
I decided to leave these points unanswered here so we don't get distracted here from the consistency fix.
@camilamacedo86 could you please submit a PR or create an issue to address this further? Please describe the issues you are facing in as much detail as possible so we can discuss it there. If it is a time consuming PR - we should probably start with an issue describing a potential solution.