Skip to content

Remove podman binary detection from podman-env command #21259

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Remove podman binary detection from podman-env command #21259

wants to merge 7 commits into from

Conversation

elasticdotventures
Copy link

Addresses feedback from reviewers by removing podman binary detection and clarifying the Docker client connectivity approach.

Key Changes

  • Removed podman binary detection: Eliminates isPodmanAvailable() function and which podman checks per reviewer feedback
  • Clarified service description: Now explicitly refers to "Podman Docker-compatible service" for precision
  • Removed version specificity: Changed from "podman 4.9.2" requirement to "recent Podman version with Docker API compatibility"
  • Focused on Docker client only: Documentation emphasizes only Docker client required, not podman binary

Addresses Review Feedback

  • ✅ @afbjorklund: Removed podman detection, focuses purely on Docker socket connectivity
  • ✅ Version specificity: Removed specific version requirements as requested
  • ✅ Cleaner approach: No podman binary dependency, only Docker client against Docker-compatible API

Before/After

Before

# Command would check for podman binary availability
eval $(minikube podman-env)  # Could fail if podman binary not found

After

# No podman binary check - only connects Docker client to Podman's Docker-compatible service
eval $(minikube podman-env)
export DOCKER_HOST="tcp://192.168.49.2:2376" 
docker images   # Works with Docker client against Podman service

Foundation for Issue #9229

This implementation provides a clean foundation for the future SSH socket approach described in issue #9229, but keeps the current changes minimal and focused. The SSH socket implementation (#9229) would be a separate, more substantial project that can build on this foundation.

Testing

  • Updated tests to remove podman detection expectations
  • All existing Docker client functionality preserved
  • Documentation updated to reflect new approach

This approach is more aligned with issue #9229's vision but represents a focused change that can be merged independently before tackling the larger SSH socket project.

Supersedes PR #21054 - This PR contains the same functionality with reviewer feedback addressed.

@elasticdotventures
dockerEnv vs podmanEnv detection
a1f2849
@elasticdotventures
Remove podman detection from docker-env per review feedback
7723b9f 
- Remove DockerBackendDetector and DetectDockerBackend function
- Remove podman-specific logic from dockerEnvVars
- Simplify docker-env to only handle Docker API compatibility
@elasticdotventures
Refactor podman-env to use Docker API compatibility per review feedback
2f07bfd 
- Change podman-env to use Docker client against Podman's Docker-compatible socket
- Remove SSH-based connectivity (Docker client doesn't support SSH keys)
- Simplify podman-env to use standard Docker environment variables
- Update tests to match new Docker API compatibility approach
- Update documentation to clarify Docker client usage
- Remove podman-specific test cases from docker-env_test.go

This addresses the core review feedback about API compatibility issues
and provides a cleaner separation between docker-env and podman-env.
@elasticdotventures
Fix whitespace in docker-env files
9457fa1 
Remove unnecessary blank lines that were accidentally introduced
when removing podman detection logic.
@elasticdotventures
Add Docker runtime support to podman-env command
0dda539 
Extend podman-env compatibility to support both crio and docker container
runtimes, providing users with more deployment flexibility while maintaining
the core Docker API compatibility approach that eliminates API version
conflicts.

This change allows podman-env to work with:
- Podman driver + crio runtime (original support)
- Podman driver + docker runtime (new support)

Both configurations use the same Docker API compatibility approach that
was implemented to address reviewer feedback about SSH-based connectivity
issues and API version mismatches.
@elasticdotventures
Update podman-env documentation with local development example
bfcdf26 
Add practical example showing how to build images directly in minikube
and deploy them to Kubernetes without needing a separate registry.
This demonstrates the key value proposition of the Docker API
compatibility approach.
@elasticdotventures
Address reviewer feedback: remove podman binary detection
5b54d5b 
- Remove isPodmanAvailable() function and related checks
- Update language to clarify "Podman Docker-compatible service"
- Remove version-specific requirements (4.9.2 -> recent version)
- Focus on Docker client connectivity without podman binary dependency

This addresses @afbjorklund's feedback about not detecting podman binaries
and providing only Docker socket connectivity. Prepares foundation for
future SSH socket implementation per issue kubernetes#9229.
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 7, 2025
@k8s-ci-robot
Copy link
Contributor

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

  • 5b54d5b Address reviewer feedback: remove podman binary detection

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 7, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @elasticdotventures. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 7, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: elasticdotventures
Once this PR has been reviewed and has the lgtm label, please assign medyagh for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@elasticdotventures
Copy link
Author

Closing this PR - will update the original PR #21054 instead to preserve discussion context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ComradeProgrammer ComradeProgrammer Awaiting requested review from ComradeProgrammer

@prezha prezha Awaiting requested review from prezha

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elasticdotventures @k8s-ci-robot @minikube-bot