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

Add podman-env for connecting with podman-remote #6351

Merged
merged 7 commits into from
Feb 11, 2020

Conversation

afbjorklund
Copy link
Collaborator

For #6350

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2020
@codecov-io
Copy link

codecov-io commented Jan 20, 2020

Codecov Report

Merging #6351 into master will decrease coverage by 0.53%.
The diff coverage is 34.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6351      +/-   ##
==========================================
- Coverage    38.1%   37.56%   -0.54%     
==========================================
  Files         137      138       +1     
  Lines        8690     8780      +90     
==========================================
- Hits         3311     3298      -13     
- Misses       4963     5066     +103     
  Partials      416      416
Impacted Files Coverage Δ
cmd/minikube/cmd/root.go 55.33% <100%> (+0.29%) ⬆️
cmd/minikube/cmd/podman-env.go 29.03% <29.03%> (ø)
cmd/minikube/cmd/docker-env.go 39.83% <51.72%> (ø)
pkg/minikube/cluster/fix.go 32.33% <0%> (-8.21%) ⬇️
cmd/minikube/cmd/start.go 22.53% <0%> (-0.07%) ⬇️
pkg/minikube/cluster/status.go 42.42% <0%> (+6.06%) ⬆️
pkg/minikube/cluster/delete.go 45.83% <0%> (+8.33%) ⬆️

@medyagh
Copy link
Member

medyagh commented Jan 21, 2020

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 21, 2020
@minikube-pr-bot
Copy link

All Times minikube: [ 243.934374 246.038455 226.580451]
All Times Minikube (PR 6351): [ 245.800417 242.165363 224.590050]

Average Minikube (PR 6351): 237.518610
Average minikube: 238.851093

Averages Time Per Log

+----------------------+------------+--------------------+
|         LOG          |  MINIKUBE  | MINIKUBE (PR 6351) |
+----------------------+------------+--------------------+
| minikube v           |   0.288504 |           0.322849 |
| Creating kvm2        | 161.176409 |         160.948650 |
| Preparing Kubernetes |  41.521624 |          39.638282 |
| Pulling images       |   2.192963 |           2.482109 |
| Launching Kubernetes |  30.731536 |          31.521425 |
| Waiting for cluster  |   2.887217 |           2.553926 |
+----------------------+------------+--------------------+

@tstromberg
Copy link
Contributor

Part of me wishes we had a vendor-neutral command we could point users to, but I think any terminology we use might be lost on them. docker-env & podman-env are likely more meaningful than any alternative we come up with.

@tstromberg
Copy link
Contributor

Merge conflict :(

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2020
@afbjorklund
Copy link
Collaborator Author

I’m not sure env --container-runtime=docker would have really improved the situation ? But that was the original machine command...

@gbraad
Copy link
Contributor

gbraad commented Feb 6, 2020

docker-env & podman-env

you could call it runtime-env... we have considered the same. it is in line with seting the runtime option. but just not sure if this is preferred. just wondering, does the code error when docker-env is used while the runtime is podman? (and vice versa)

@afbjorklund
Copy link
Collaborator Author

just wondering, does the code error when docker-env is used while the runtime is podman? (and vice versa)

There will be an error if docker is not running or podman is not installed, but otherwise you're free to choose...

$ minikube docker-env
💣  The docker service is currently not active
$  ./out/minikube podman-env
💣  The podman service is currently not available

It is quite common to use docker to build images for containerd, or to use podman to build images for cri-o.

@afbjorklund
Copy link
Collaborator Author

I will have to rewrite this

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 9, 2020
@afbjorklund
Copy link
Collaborator Author

I moved the tests from the pkg (generic) to the cmd (specific), as per the new code at master.

There were a lot of symbol conflicts with "env", so had to prefix a lot of things (miss my static)

@medyagh
Copy link
Member

medyagh commented Feb 10, 2020

@afbjorklund any chance we could have integraiton test for this or documentation how to use it ?

@afbjorklund
Copy link
Collaborator Author

@medyagh : normally I try to write the user-facing documentation in the issue (not the PR)

Included a doc page: https://deploy-preview-6351--kubernetes-sigs-minikube.netlify.com/docs/tasks/podman_service/

@afbjorklund
Copy link
Collaborator Author

Stuck on lint

pkg/minikube/shell/shell.go:85:2: var `unset` is unused (unused)
cmd/minikube/cmd/podman-env.go:216:60: podmanEnvVars - result 1 (error) is always nil (unparam)

@minikube-pr-bot
Copy link

All Times minikube: [ 92.450157 91.846553 94.694479]
All Times Minikube (PR 6351): [ 92.005527 94.129682 93.452711]

Average Minikube (PR 6351): 93.195973
Average minikube: 92.997063

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6351) |
+----------------------+-----------+--------------------+
| minikube v           |  0.205016 |           0.196737 |
| Creating kvm2        | 19.998522 |          19.963837 |
| Preparing Kubernetes | 50.250073 |          49.914298 |
| Pulling images       |           |                    |
| Launching Kubernetes | 20.648621 |          21.162565 |
| Waiting for cluster  |  0.231623 |           0.249830 |
+----------------------+-----------+--------------------+

@medyagh medyagh merged commit ff763db into kubernetes:master Feb 11, 2020
@laurent-indermuehle
Copy link

Hi @afbjorklund, Thanks for your work!
I landed here because I was reading this: https://minikube.sigs.k8s.io/docs/tasks/podman_service/
But I got "Error: unknown command "podman-env" for "minikube".
It seems to be because this PR is merged but not included in the last release (1.7.2). Right?

If it's the case, it may be a good idea to release the doc at the same time as the code. Is it feasible?

@afbjorklund
Copy link
Collaborator Author

It should be in 1.7.3 now, unfortunately (?) the docs are showing master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants