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: remove hardcoded docker occurences. #23

Merged
merged 7 commits into from
May 17, 2023

Conversation

trusch
Copy link
Contributor

@trusch trusch commented Apr 27, 2023

This PR removes two hardcoded occurences of the docker container engine. Since a few weeks, users can choose to use podman instead of docker, but in some places "docker" was still not made dynamic.

fixes #22

@chevdor
Copy link
Owner

chevdor commented Apr 27, 2023

This is a very welcome fix. Thank you.

I also think it would be good in general to switch the default to Podman but I want to make sure we do not suprise the users who may not have a Podman install ready to be used. Ideally, the doc should be reworked to mention something more neutral than Docker and use the word "containers" instead.

Quick option

I am thinking that we could drop a warning if the engine is set to docker and mention that the default will be switched to podman in the next release(s) so users are invited to use or at least test the podman engine already.

Smarter option

As alternative, we could be smart about it and check whether podman and docker are installed and use as engine:

  • only podman detected => podman
  • only docker detected => docker + warning
  • both => whatever the user select but switch the default to podman abd warn the user if they picked docker

In term of detection (I ran into this recently), we need to make sure we don't stop at just finding the "command".
I, for instance, use an alias docker=podman, since I still end up typiong docker by reflex from time to time.

Using --version whill give away what the engine really is:

docker --version   
podman version 4.5.0

The above could be done in this PR or in another one. Your pick :)

Copy link
Owner

@chevdor chevdor left a comment

Choose a reason for hiding this comment

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

See comments

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
@chevdor chevdor mentioned this pull request May 2, 2023
3 tasks
@trusch
Copy link
Contributor Author

trusch commented May 12, 2023

@chevdor Sorry that it took a while, but I had stuff to do 🤷‍♂️ Hope you are happy with the result!

@chevdor
Copy link
Owner

chevdor commented May 12, 2023

No stress and thanks for the work!
I am not sure I can check it today but I will have a look asap.

@trusch trusch requested a review from chevdor May 16, 2023 12:41
- move the ContainerEngine related fn to the appropriate file
- switch the engine type type to ContainerEngine (String formerly) and add value_parser
- error cleanup
- set --no-cache by default for Podman
- add doc
Copy link
Owner

@chevdor chevdor left a comment

Choose a reason for hiding this comment

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

I included a few fixes directly

@chevdor chevdor merged commit 19c56fe into chevdor:master May 17, 2023
1 check 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.

Exit handler has hardcoded container engine
2 participants