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

pod support #6

Merged
merged 2 commits into from
Oct 16, 2024
Merged

pod support #6

merged 2 commits into from
Oct 16, 2024

Conversation

kai-tub
Copy link
Contributor

@kai-tub kai-tub commented Sep 18, 2024

Hey 👋

Thanks for creating this awesome nix wrapper around quadlet! 🙏

I have added pod support and tried to use the same style that was used in container.nix.
I also added some evaluation tests to ensure that the names remain unique because I kept accidentally giving the pod and container the same name during tests.

One thing that I am unsure about is whether or not to enforce the .pod suffix in the option to remain consistent with the upstream podman documentation, as they require <name>.pod. This leads to the somewhat weird example:

{
    # ...
    virtualisation.quadlet = {
        containers = {
            nginx.containerConfig.image = "docker.io/library/nginx:latest";
            nginx.containerConfig.networks = [ "host" "internal.network" ];
            nginx.containerConfig.pod = "nginx-pod.pod";
            nginx.serviceConfig.TimeoutStartSec = "60";
        };
        networks = {
            internal.networkConfig.subnets = [ "10.0.123.1/24" ];
        };
        pods = {
          nginx-pod = { };
        };
    };
}

But to provide a better UX, I have also added an assertion that ensures that these pod options end with .pod and otherwise reminds the developer to add the suffix if necessary.

I have also applied the official formatter, mostly because I forgot to disable automatic formatting for the repository. 😅

I haven't tried all options yet but I was wondering how the testing strategy should look like. Should I try to add nixos tests for each configuration or is it fine to start with a "manual" check and then fix things as they appear/improve upon the testing later on?

Thanks!

Copy link
Owner

@SEIAROTg SEIAROTg left a comment

Choose a reason for hiding this comment

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

Thanks for contributing!

LGTM in general with some minor comments.

Re formatting: LGTM, but for clean history can you format the current master and rebase your change onto it? That way we see one pure format commit and one pure functional commit.

Re testing: This was initially just me sharing my personal snippet so there isn't a good testing story for now. I'll probably set up some proper testing going forward but for this one I'll just merge with some manual testing once all comments are resolved. Thanks for highlighting that.

Also can you update README.md to reflect that pod is now supported?

nixos-module.nix Outdated Show resolved Hide resolved
nixos-module.nix Outdated Show resolved Hide resolved
nixos-module.nix Outdated Show resolved Hide resolved
nixos-module.nix Outdated Show resolved Hide resolved
@kai-tub
Copy link
Contributor Author

kai-tub commented Sep 23, 2024

Re formatting: LGTM, but for clean history can you format the current master and rebase your change onto it? That way we see one pure format commit and one pure functional commit.

Geez, that was a challenging git exercise for me 😅
I hope that it is correct now.

Re testing: This was initially just me sharing my personal snippet so there isn't a good testing story for now. I'll probably set up some proper testing going forward but for this one I'll just merge with some manual testing once all comments are resolved. Thanks for highlighting that.

Sure, I totally get it. 👍 It is nice first to get a feel for how it should "feel" before spending way too much time on testing infrastructure.

@SEIAROTg
Copy link
Owner

Did some manual test and everything looks good. Thanks!

Before I merge, would you mind rebasing to the current head so I can fast-foward with your signed commits?

Include some basic tests to ensure that the container + pod
configurations is valid.
Add test that ensures that the names for containers/pods/networks is
unique to avoid cryptic error messages.
@kai-tub
Copy link
Contributor Author

kai-tub commented Oct 16, 2024

Rebased 👍

@SEIAROTg SEIAROTg merged commit 5970e7b into SEIAROTg:main Oct 16, 2024
@kai-tub kai-tub changed the title WIP: pod support pod support Oct 27, 2024
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.

2 participants