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: Read pod name from podman create args #89

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

ananthb
Copy link
Contributor

@ananthb ananthb commented Jun 25, 2024

Fixes #88

@ananthb
Copy link
Contributor Author

ananthb commented Jun 26, 2024

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
Copy link
Contributor Author

ananthb commented Jul 8, 2024

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
Copy link
Contributor Author

ananthb commented Jul 8, 2024

This seems relevant: clap-rs/clap#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
Copy link
Contributor Author

ananthb commented Jul 9, 2024

I've gotten this far. Stuck here:

Screenshot 2024-07-09 at 12 39 53

@k9withabone
Copy link
Member

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
Copy link
Contributor Author

ananthb commented Sep 20, 2024

I think I've managed to fix it now.

@ananthb ananthb force-pushed the pod-name branch 4 times, most recently from 3750115 to 069db99 Compare September 21, 2024 07:17
Only set `PodName=` Quadlet option when `--name` is used per the
discussion in containers#88.

Renamed `name` to `name_flag`.

Moved `name_flag` so that it matches the `PodName=` position on
podman-systemd.unit(5) man page.

Added more user-friendly documentation to `name_flag` help text.

Set `long = "name"` and `short` arg attributes for `name_flag`.

Refactored `Pod::name()` a bit to use more destructuring and
`Option::as_deref()`.

Signed-off-by: Paul Nettleton <[email protected]>
@k9withabone
Copy link
Member

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!

@k9withabone k9withabone added this to the v0.3.1 milestone Sep 23, 2024
@k9withabone k9withabone added bug Something isn't working enhancement New feature or request labels Sep 23, 2024
@ananthb
Copy link
Contributor Author

ananthb commented Sep 23, 2024

Sounds good to me.

@k9withabone k9withabone merged commit 5356c2f into containers:main Sep 24, 2024
13 checks passed
@ananthb ananthb deleted the pod-name branch September 24, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support podman pod create --name option
2 participants