containers / podlet

Generate Podman Quadlet files from a Podman command, compose file, or existing object
https://crates.io/crates/podlet
Mozilla Public License 2.0
422 stars 12 forks source link

fix: Read pod name from podman create args #89

Closed ananthb closed 1 week ago

ananthb commented 3 months ago

Fixes #88

ananthb commented 3 months ago

Essentially this reads the --name=... arg but ignores it right? I'm brand new to Rust but that's what it looks like to me.

Is that okay? It still worked fine for me.

I tried using the name in the impl as well and that led to the same output.

ananthb commented 2 months ago

This change seems to break the already working functionality where the pod name is read from the arg instead.

Screenshot 2024-07-08 at 21 53 15
ananthb commented 2 months ago

This seems relevant: https://github.com/clap-rs/clap/issues/1820

I'm not sure how I'd apply that to this codebase because no one on that thread is using the attribute syntax we're using here.

ananthb commented 2 months ago

I've gotten this far. Stuck here:

Screenshot 2024-07-09 at 12 39 53
k9withabone commented 2 weeks ago

Thanks for working on this! You are getting that error because required is being set implicitly by the derive macro, see the Arg Types table in the clap derive reference. You need to change the types of both name and name_pos to Option<String>. Then, on name, I think you should change the required_unless_pressent to conflicts_with. Finally, in the impl From<Create> for quadlet::Pod, you can use something like

let pod_name = name_pos.or(name).expect("`name` or `name_pos` is required");

Small nitpicks: I prefer identifiers not to be shortened, so if you could rename name_pos to name_positional and name to name_flag (then use long = "name" in the attribute) to make it less confusing. Also, could you rearrange the ordering so that the positional is at the end of the Create struct and copy the doc comments from name_positional to name_flag, maybe also adding something about using only the flag or the posiitonal?

ananthb commented 1 week ago

I think I've managed to fix it now.

k9withabone commented 1 week ago

Made it so that the PodName= Quadlet option is only set when --name is used per the discussion in #88. Also, a few other small changes. I think it's good to go for merging if you agree. Thanks again for your work on this!

ananthb commented 1 week ago

Sounds good to me.