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

podman: install package if enabled and create config files #6039

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dawidd6
Copy link
Contributor

@dawidd6 dawidd6 commented Nov 4, 2024

Description

Copied some useful options from NixOS podman module and adapted the configuration to home-manager.

Also setting services.podman.enable = true will now add podman to PATH.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

@bamhm182 @n-hass

@bamhm182
Copy link

bamhm182 commented Nov 4, 2024

I like it and it looks mostly good. For some reason ubuntu-latest is failing here, but they seem unrelated and I don't seem to be getting them locally. That said, it does appear that the actual output of registries.conf has an extra [registries] section at the top that shouldn't be there.

@n-hass
Copy link
Contributor

n-hass commented Nov 5, 2024

LGTM.
If you wanted to avoid the name clashing with actual resource definitions, you could scope these configuration options under config like services.podman.config.xx

@bamhm182 registries.conf default settings gives this, which should be fine

[registries.block]
registries = []

[registries.insecure]
registries = []

[registries.search]
registries = ["docker.io"]

@bamhm182
Copy link

bamhm182 commented Nov 5, 2024

I just had a few spare minutes yesterday, so I opted to clone the repo and run the tests while reviewing it and hadn't had time to dig deep into it. I spun up a new VM to mess with it, and I can't recreate the issue I was seeing in the test, but the test still fails the same way. This is what I am seeing on a few different machines.

$ nix develop --ignore-environment .#podman-configuration
podman-configuration: FAILED
Expected /nix/store/w6qihr88hmn3rd8wck2hra465rqbn36m-nmt-report-podman-configuration/normalized/registries.conf to be same as /nix/store/7sn4505w29lgmkwny52s9nqsm6vf4yp7-configuration-registries-expected.conf but were different:
--- actual
+++ expected
@@ -1,4 +1,3 @@
-[registries]
 [registries.block]
 registries = ["ghcr.io", "gallery.ecr.aws"]

For further reference please introspect /nix/store/w6qihr88hmn3rd8wck2hra465rqbn36m-nmt-report-podman-configuration

Looking into it further, it has to do with using nix develop for the tests as nix-shell --pure tests -A run.podman-configuration appears to work just fine.

@dawidd6
Copy link
Contributor Author

dawidd6 commented Nov 5, 2024

Hmm. Different nixpkgs revisions? I was using latest master for the evaluation using nix-shell.

@bamhm182
Copy link

bamhm182 commented Nov 5, 2024

Just went through and made sure that everything I could find was up to date. Now my test results match those of the automatic pipeline. Sorry for the confusion!

@n-hass, I'm not totally sure I understand what that suggestion is trying to avoid. Where would we run into clashes with the implementation by dawidd6?

@n-hass
Copy link
Contributor

n-hass commented Nov 5, 2024

Where would we run into clashes with the implementation by dawidd6?

Eg having to name services.podman.containersConf because there is already services.podman.containers. Actual resources vs podman configuration files.

@bamhm182
Copy link

bamhm182 commented Nov 6, 2024

Ah, yeah. I see what you're saying. I'm also wondering if this belongs under the podman configuration, or if it ought to be under a services.containers like it is for the main nixpkgs. I know right now there's only podman in HM, but rootless docker is technically a thing and uses the same files.

@bamhm182
Copy link

bamhm182 commented Nov 8, 2024

Went ahead and submitted a PR on @dawidd6's fork here. I just put everything under services.podman.config as @n-hass had suggested.

@dawidd6
Copy link
Contributor Author

dawidd6 commented Nov 8, 2024

Isn't it a little too deep now? services.podman.config.storage.settings, maybe only services.podman.config.storage would suffice?

@bamhm182
Copy link

bamhm182 commented Nov 8, 2024

Yeah. Very fair point. Made the change to the PR I made.

EDIT: Just realized you'd already merged. Want to just make the change yourself?

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.

3 participants