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

🐛 More needed for podman workaround in kind-load (take 2) #794

Closed

Conversation

bentito
Copy link
Contributor

@bentito bentito commented Apr 26, 2024

The commit adding the podman workaround for the kind-load target seems to need a bit more work:

It needs to have the tag name set before image-archive is saved

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

The commit adding the podman workaround for the kind-load target seems to need a bit more work:

It needs to have the tag name set before image-archive is saved

Signed-off-by: Brett Tofel <btofel@redhat.com>
@bentito bentito requested a review from a team as a code owner April 26, 2024 19:39
Copy link

netlify bot commented Apr 26, 2024

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit 6c98d7d
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/662c02e77d157b00080fba27
😎 Deploy Preview https://deploy-preview-794--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Apr 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.21%. Comparing base (712cd04) to head (6c98d7d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #794   +/-   ##
=======================================
  Coverage   67.21%   67.21%           
=======================================
  Files          23       23           
  Lines        1467     1467           
=======================================
  Hits          986      986           
  Misses        415      415           
  Partials       66       66           
Flag Coverage Δ
e2e 45.53% <ø> (ø)
unit 61.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@bentito
Copy link
Contributor Author

bentito commented Apr 29, 2024

/assign @perdasilva

Comment on lines +156 to +162
@DEPLOY_TEMPLATE_IMG_NAME=quay.io/operator-framework/operator-controller:devel; \
TMP_FILE=temp_image.tar; \
podman tag $(IMG) $$DEPLOY_TEMPLATE_IMG_NAME; \
echo "Saving image to temporary file $$TMP_FILE"; \
podman save $$DEPLOY_TEMPLATE_IMG_NAME -o $$TMP_FILE; \
$(KIND) load image-archive $$TMP_FILE --name $(KIND_CLUSTER_NAME); \
rm $$TMP_FILE
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing a completely different thing for podman/docker, could we just do this exact procedure for both, where we just use $(CONTAINER_RUNTIME) instead of hardcoding docker or podman?

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Meta-comment:

It is really difficult to support every permutation of developer environment, especially if we don't centralize these sorts of task definitions. Whatever we decide here would need to be duplicated in basically all of the active repos and then maintained.

If we don't test each configuration in CI, then they are extremely prone to rot, because the only one guaranteed to work is the one used in CI. If we do test each configuration, then CI becomes more expensive AND it is hard for a developer making this kind of change to fix something about an environment they don't have setup locally.

IMO, if at all possible, it is better for the repo to be very opinionated on the environment and to have developers adapt to the repo instead of vice versa

Even Mac vs Linux is hard enough (and we don't explicitly test Mac in CI). With podman support, the matrix becomes linux/docker, linux/podman, mac/docker, mac/podman.

@m1kola
Copy link
Member

m1kola commented Apr 29, 2024

Could you please explain the issue in more detail? I'm using podman on Mac and do not observe issues with our Makefile. But to be fair - instead of passing CONTAINER_RUNTIME into make I have a symlink from docker to podman for convenience.

@bentito
Copy link
Contributor Author

bentito commented Apr 29, 2024

Meta-comment:

It is really difficult to support every permutation of developer environment, especially if we don't centralize these sorts of task definitions. Whatever we decide here would need to be duplicated in basically all of the active repos and then maintained.

If we don't test each configuration in CI, then they are extremely prone to rot, because the only one guaranteed to work is the one used in CI. If we do test each configuration, then CI becomes more expensive AND it is hard for a developer making this kind of change to fix something about an environment they don't have setup locally.

IMO, if at all possible, it is better for the repo to be very opinionated on the environment and to have developers adapt to the repo instead of vice versa

Even Mac vs Linux is hard enough (and we don't explicitly test Mac in CI). With podman support, the matrix becomes linux/docker, linux/podman, mac/docker, mac/podman.

I'd argue our time is more expensive than CI time, most of us have Macs for local dev and I feel like we should be trying to use Podman if its diffs from Docker don't get in our way too badly. Also, CI likely is closer to Podman than Docker internally anyway because CRI-O is using buildah under the hood. If we can't add this conditional to make Podman easy and cheaper for devs to use, I guess I'll work out how to auto-apply/delete before commit these kind of things so I can personally work better, but they're in conditionals, does it matter if the podman version is found to have rotted later? It can get fixed if someone is motivated and all can benefit in practice.

@bentito
Copy link
Contributor Author

bentito commented Apr 29, 2024

Could you please explain the issue in more detail? I'm using podman on Mac and do not observe issues with our Makefile. But to be fair - instead of passing CONTAINER_RUNTIME into make I have a symlink from docker to podman for convenience.

Ah, well your symlink comment made me go try it. Sure enough the problem here is not what I thought. I thought podman didn't properly support loading a docker-image and thus needed a workaround. But once I added the symlink it's obvious that kind load simply needs a "docker" in the path to work.

Closing this PR, thanks @m1kola

@bentito bentito closed this Apr 29, 2024
@m1kola
Copy link
Member

m1kola commented Apr 29, 2024

I looked a bit into this - it seems to be a known issue:

@bentito bentito deleted the kind-podman-fix-more branch May 3, 2024 18:46
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.

None yet

4 participants