Skip to content
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

🌱 fix: update kind-load target to ensure that works aligned with controller-operator #462

Merged

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Nov 18, 2024

Standardizing this method simplifies workflows and makes the project more contributor-friendly

@camilamacedo86 camilamacedo86 requested a review from a team as a code owner November 18, 2024 09:29
Copy link

codecov bot commented Nov 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.23%. Comparing base (dd64145) to head (c9c3807).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #462   +/-   ##
=======================================
  Coverage   38.23%   38.23%           
=======================================
  Files          15       15           
  Lines        1224     1224           
=======================================
  Hits          468      468           
  Misses        706      706           
  Partials       50       50           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@camilamacedo86 camilamacedo86 changed the title 🐛 fix: update kind-load target to support local kind clusters 🌱 fix: update kind-load target to support local kind clusters Nov 18, 2024
@@ -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
$(KIND) export kubeconfig --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME)
docker save $(IMAGE) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
Copy link
Member

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.

  • This change ensures that the kind-load target works seamlessly with any local kind cluster created by the developer, without relying on a bingo-provided version of kind.

What did you mean here? Was the intention to use kind from PATH? 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.

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Nov 18, 2024

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.

⚠️ I also don’t want to remember or use /Users/camilam/go/bin/kind-v0.24.0 to 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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:

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.

This change does not achieve that: make still uses bingo versioned kind. Don't hesitate to submit a PR to further address this.

Copy link
Contributor Author

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

Copy link
Member

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.

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:

  • for a contributor to use alternate versions of the build tools that are specified by the repo
  • for maintainers to increase the complexity of the Makefile to support it.

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.

Copy link
Member

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.

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

I'm happy to approve this PR, but commit message needs updating: in this commit we still use bingo provided kind version so this is misleading.

Let's update the commit message, merge and address everything else separately.

…ller-operator

Standardizing this method simplifies workflows and makes the project more contributor-friendly
@camilamacedo86 camilamacedo86 changed the title 🌱 fix: update kind-load target to support local kind clusters 🌱 fix: update kind-load target to ensure that works aligned with controller-operator Nov 18, 2024
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Nov 18, 2024

HI @m1kola

Thank you for being a good catcher. I changed the message you are right
For now, let's move with working as con-oper
for we unblock #464 and allow me to run it locally
see that as it is now, it fails for me (I executed make run to have the following result):

 $ kubectl get all -n olmv1-system
NAME                                               READY   STATUS             RESTARTS     AGE
pod/catalogd-controller-manager-68544b985f-vwjz9   0/1     CrashLoopBackOff   1 (7s ago)   16s

NAME                       TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)                     AGE
service/catalogd-service   ClusterIP   10.96.212.219   <none>        443/TCP,9443/TCP,7443/TCP   16s

NAME                                          READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/catalogd-controller-manager   0/1     1            0           16s

NAME                                                     DESIRED   CURRENT   READY   AGE
replicaset.apps/catalogd-controller-manager-68544b985f   1         1         0       16s

Then, I will work on a follow-up to remove the KIND from bingo in both projects.

@@ -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
$(KIND) export kubeconfig --name $(KIND_CLUSTER_NAME)
$(KIND) load docker-image $(IMAGE) --name $(KIND_CLUSTER_NAME)
docker save $(IMAGE) | $(KIND) load image-archive /dev/stdin --name $(KIND_CLUSTER_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why replace common call to Kind with specific call to a particular container-runtime?

Copy link
Member

@m1kola m1kola Nov 18, 2024

Choose a reason for hiding this comment

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

@bentito there is currently no support for CONTAINER_RUNTIME variable in Makefile in catalogd. @camilamacedo86 addresses this in #464 which is currently waiting for this PR.

@perdasilva perdasilva added this pull request to the merge queue Nov 18, 2024
Merged via the queue into operator-framework:main with commit 55689e8 Nov 18, 2024
15 checks passed
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.

5 participants