-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Podman script addition #188
base: development
Are you sure you want to change the base?
Conversation
Cleaning up afterwards will be nice but I'm not sure how to do that yet (or re-using the existing container/volume) |
We probably don't want to clean-up, at least not the Nix store volume, as it reduces build time for further invocations. Though it might be relevant to document how to clean it up? Can someone who knows podman also review and chime in? Thanks! |
What's your reason for making an entirely different script rather than just changing the 2 Docker commands in the existing script? You can just detect if Podman is installed and if so prefer that one, including the extra command to check for existing volumes. |
@PureTryOut , separating the two at least for now seems like the safest approach. I don't want to break something that is already working. I don't know how readable it would be to hide what is used behind another variable (or how that var would be named). Although it might have the benefit of passing info to the user early as of what can be used 🤔 Note: I'm sure there is more that can be improved but it's a start. |
Alright, complete revamp of the previous iteration, @samueldr @PureTryOut - now both Added:
Things which I'm uncertain if I implemented correctly:
Other:
Nice to have but most likely for a different PR:
|
# Check if docker is available | ||
if ! command -v docker &> /dev/null; | ||
then | ||
stderr.printf "Docker not installed\n" | ||
else | ||
availableManagers+=('docker') | ||
fi | ||
|
||
|
||
# Check if podman is available | ||
if ! command -v podman &> /dev/null; | ||
then | ||
stderr.printf "Podman not installed\n" | ||
else | ||
availableManagers+=('podman') | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to add to unavailableManagers
and print the list of unavailable managers when availableManagers
is empty.
Otherwise we have a spurious non-actionable warning/logging printed.
Or just add to availableManagers
in an if
without else and not log it, since the error message currently lists the supported options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this behave okay in cases where docker is symlinked to podman? As far as I'm aware it's not uncommon to have a docker executable that just calls podman.
local selectedManager | ||
# Use switch statements to select from the available managers | ||
stderr.printf "\nAvailable container managers on your system:\n" | ||
PS3="Select one of the options or quit: " | ||
select manager in ${availableManagers[*]}; do | ||
case $manager in | ||
docker|podman) | ||
stderr.printf "\nSelected:\n $manager\n" | ||
selectedManager=$manager | ||
break | ||
;; | ||
Quit) | ||
stderr.printf "\nSee you soon!\n" | ||
exit 0 | ||
;; | ||
*) | ||
stderr.printf "\nInvalid option\n" | ||
;; | ||
esac | ||
done | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this right, this will always stop and ask, right?
This would break the current semantics of acting like a "thick" wrapper to nix-build
if I got this right.
We could prefer the first in availableManagers
, if some environment variable is not set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to check how to expand this functionality.
I'm thinking of "interactive" mode where the user can do the selections from above.
But to be executed without any required interaction by default.
# Pull the image if it doesn't exists | ||
if (( $checkImg == 0 )); | ||
then | ||
stderr.printf "\nNixOS image is present\n" | ||
elif (( $checkImg == 1 )); | ||
then | ||
stderr.printf "\nCan\'t find NixOS image. Pulling...\n" | ||
$selectedManager pull docker.io/nixos/nix | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's the nix
image, hosted under the NixOS organization. I guess to make this easier to reason about, always printing nixos/nix
when referring to the image is better, since that is the identifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change it to match the expected image.
select yn in "Yes" "No"; do | ||
case $yn in | ||
Yes) | ||
stderr.printf "\nProceeding as root user\n" | ||
break | ||
;; | ||
No) | ||
stderr.printf "\nQuiting the build script...\n" | ||
exit 0 | ||
;; | ||
esac | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is a case where stopping and asking the user is probably fine? I don't know enough about docker/podman and friends to know if there are drawbacks that warrant stopping running as root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it would ideally be a good spot to always request permission, most Docker && Podman starter guides are for root setups.
Will change it to always print something visible. But remove the forced interaction by default.
Sorry it took this long until I could take a look. I left some notes about the script. Though it's possible I missed things. Also note I haven't ran it yet. One main consideration with the script is it should never* stop and ask things. Otherwise it'll differ too greatly from the usual *: except maybe for that thing about running as root, I don't know. |
Thank you for the great feedback @samueldr ! At the top of my head, perhaps an interactive flag could be a solution. It would allow a flexible route when desired by the user while adhering to the "no-stop to ask" rule as a default. Most guides for starting with Docker and Podman don't add a mention for rootless mode. Most likely to lower the barrier for entry. |
- Shorter sentences for larger output blocks - Provide more accurate information in the output Co-authored-by: Samuel Dionne-Riel <samuel@dionne-riel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like progress on this has stalled, would it be helpful if I open up a new pr with a simplified version of this?
# Check if docker is available | ||
if ! command -v docker &> /dev/null; | ||
then | ||
stderr.printf "Docker not installed\n" | ||
else | ||
availableManagers+=('docker') | ||
fi | ||
|
||
|
||
# Check if podman is available | ||
if ! command -v podman &> /dev/null; | ||
then | ||
stderr.printf "Podman not installed\n" | ||
else | ||
availableManagers+=('podman') | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this behave okay in cases where docker is symlinked to podman? As far as I'm aware it's not uncommon to have a docker executable that just calls podman.
Basic Podman helpers:
Can be executed in rootless mode following the Podman docs: https://podman.io/getting-started/installation#linux-distributions
No changes made to the existing Docker script