fix(3197): fix youki version command Part of Enhancing Compatibility with runc by tommady · Pull Request #3200 · youki-dev/youki

tommady · 2025-07-20T01:52:46Z

Description

Fix Youki version command
This is Part of Enhancing Compatibility with runc

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Refactoring (no functional changes)
Performance improvement
Test updates
CI/CD related changes
Other (please describe):

Testing

Added new unit tests
Added new integration tests
Ran existing test suite
Tested manually (please provide steps)

./target/debug/youki -v                                                                                                                                  
youki version: 0.5.4                                                                                                                                          
commit: 0.5.4-81806f8a9175bbed7cb176cb66f527ff89a187ac

Related Issues

Fixes #3197

Additional Context

…with runc Signed-off-by: tommady <[email protected]>

Copilot

Pull Request Overview

This PR fixes the youki version command to enhance compatibility with runc by updating the version output format and command-line interface behavior. The changes ensure that youki -v or youki --version produces output in a Moby-compatible format.

Modified CLI argument parsing to handle version flag separately instead of using a macro
Updated version output format to match Moby/runc compatibility requirements
Changed subcommand handling to be optional and display help when no subcommand is provided

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
crates/youki/src/main.rs	Updated CLI parsing to add explicit version flag handling and made subcommands optional
crates/youki/src/commands/info.rs	Modified version output format to be Moby-compatible

Copilot · 2025-07-21T04:19:34Z

crates/youki/src/main.rs


    #[clap(subcommand)]
-    subcmd: SubCommand,
+    subcmd: Option<SubCommand>,


Making subcmd optional changes the CLI behavior significantly. Consider if this is the intended breaking change, as users who previously relied on subcommands being required may experience different behavior.

Suggested change

subcmd: Option<SubCommand>,

subcmd: SubCommand,

Copilot · 2025-07-21T04:19:35Z

crates/youki/src/main.rs

        }
+        None => Opts::command()
+            .print_help()
+            .map_err(|e| anyhow::anyhow!(":{e}")),


The error message format ":{e}" has an unusual leading colon. Consider using a more descriptive format like "Failed to print help: {e}".

Suggested change

.map_err(|e| anyhow::anyhow!(":{e}")),

.map_err(|e| anyhow::anyhow!("Failed to print help: {e}")),

utam0k · 2025-07-21T04:20:16Z

@saku3 May I ask you to review this P?

saku3 · 2025-07-21T05:57:01Z

Sure!

Signed-off-by: tommady <[email protected]>

saku3 · 2025-07-26T00:35:49Z

crates/youki/src/main.rs

+        Some(SubCommand::Completion(completion)) => {
            commands::completion::completion(completion, &mut app)
        }
+        None => Opts::command()


Suggested change

None => Opts::command()

None => app.print_help().map_err(|e| anyhow::anyhow!(":{e}"))

Signed-off-by: tommady <[email protected]>

saku3 · 2025-07-26T23:32:55Z

Thanks. I think this modification looks good.

crates/youki/src/commands/info.rs

utam0k · 2025-08-05T11:04:48Z

Could you add the tests to keep this compatibility with runc?

Signed-off-by: tommady <[email protected]>

tommady · 2025-08-06T11:09:56Z

Could you add the tests to keep this compatibility with runc?

sorry I am not quite sure how, could you guide me?

I thought the issue's goal is to let youki accept -v instead of -V

YJDoc2 · 2025-08-07T07:39:14Z

sorry I am not quite sure how, could you guide me?
I thought the issue's goal is to let youki accept -v instead of -V

Hey, so that is correct, utam0k is asking to add test, so we can be safe against regression with this. The test for this would be running youki with -v . So in the test we can run the youki with -v flag and make sure that it does not error, and if we want the o/p in a certain format, we can also check that. Currently the best place for this kind of test would be in the e2e tests, but this seems a bit out-of-context for that (e2e are focused on behavioral , this seems more like an interface test) . Maybe for now we can have a simple sh script like what we have for rootless podman test, and check in that?

Also do should we support -V as well for backward compatibility?
Can you add a comment in code for why we set disable_version_flag=true ?

Thanks :)

tommady · 2025-08-07T13:14:29Z

sorry I am not quite sure how, could you guide me?

I thought the issue's goal is to let youki accept -v instead of -V

Hey, so that is correct, utam0k is asking to add test, so we can be safe against regression with this. The test for this would be running youki with -v . So in the test we can run the youki with -v flag and make sure that it does not error, and if we want the o/p in a certain format, we can also check that. Currently the best place for this kind of test would be in the e2e tests, but this seems a bit out-of-context for that (e2e are focused on behavioral , this seems more like an interface test) . Maybe for now we can have a simple sh script like what we have for rootless podman test, and check in that?

Also do should we support -V as well for backward compatibility?

Can you add a comment in code for why we set disable_version_flag=true ?

Thanks :)

I think you’ve highlighted two key points:
1. Should we maintain backward compatibility?
2. What should the testing strategy be?

These two directly affect your suggestions:
1. For e2e, shell script, and interface tests, they all rely on exact output matching. Without that, we lack a clear criterion for validation. However, I believe that level of thoroughness goes beyond the scope of this issue.
2. Regarding the disable_version_flag=true comment—whether or not we add it depends on the decision about backward compatibility.
• If we decide to support backward compatibility, the comment might not be necessary.
• If we don’t, this becomes a breaking change and should be mentioned in the release notes.
• Also, since it’s a Clap-only argument, I assumed it was already documented sufficiently.

In any case, I think we should keep this PR simple and focused, as requested by the original issue.

utam0k · 2025-08-10T11:19:29Z

Should we maintain backward compatibility?

In my opinion, we don't need backward compatibility in this case. Not many people currently depend on this behavior.

What should the testing strategy be?

After reading YJDoc's comments, I reconsidered. I don't think it's necessary to test using new methods. I vote against no testing.

tommady · 2025-08-11T09:28:22Z

Should we maintain backward compatibility?

In my opinion, we don't need backward compatibility in this case. Not many people currently depend on this behavior.

What should the testing strategy be?

After reading YJDoc's comments, I reconsidered. I don't think it's necessary to test using new methods. I vote against no testing.

Do you think the manual test is OK for you?

utam0k · 2025-08-11T11:13:10Z

Do you think the manual test is OK for you?

Yes, I think, but wait for @YJDoc2

YJDoc2 · 2025-08-16T12:05:29Z

Hey, I'm also fine with manual test for this.

…with runc (youki-dev#3200) * fix(3197): fix youki version command Part of Enhancing Compatibility with runc Signed-off-by: tommady <[email protected]> * fix copilot review on print help suggestion Signed-off-by: tommady <[email protected]> * address command Signed-off-by: tommady <[email protected]> * address comment Signed-off-by: tommady <[email protected]> --------- Signed-off-by: tommady <[email protected]>

fix(3197): fix youki version command Part of Enhancing Compatibility …

81806f8

…with runc Signed-off-by: tommady <[email protected]>

utam0k requested a review from Copilot July 21, 2025 04:19

utam0k added the kind/feature label Jul 21, 2025

Copilot AI reviewed Jul 21, 2025

View reviewed changes

fix copilot review on print help suggestion

722dc5d

Signed-off-by: tommady <[email protected]>

saku3 reviewed Jul 26, 2025

View reviewed changes

address command

097142e

Signed-off-by: tommady <[email protected]>

utam0k reviewed Aug 5, 2025

View reviewed changes

crates/youki/src/commands/info.rs Show resolved Hide resolved

address comment

914ef8e

Signed-off-by: tommady <[email protected]>

utam0k added the breaking change label Aug 10, 2025

YJDoc2 approved these changes Aug 16, 2025

View reviewed changes

utam0k merged commit 8bd3f38 into youki-dev:main Aug 25, 2025
30 checks passed

github-actions bot mentioned this pull request Aug 25, 2025

Release for v0.5.6 #3227

Merged

tommady deleted the close-issue-3197 branch August 26, 2025 04:12

	.map_err(\|e\| anyhow::anyhow!(":{e}")),
	.map_err(\|e\| anyhow::anyhow!("Failed to print help: {e}")),

	None => Opts::command()
	None => app.print_help().map_err(\|e\| anyhow::anyhow!(":{e}"))

Conversation

tommady commented Jul 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Testing

Related Issues

Additional Context

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

utam0k commented Jul 21, 2025

Uh oh!

saku3 commented Jul 21, 2025

Uh oh!

saku3 Jul 26, 2025

Choose a reason for hiding this comment

Uh oh!

saku3 commented Jul 26, 2025

Uh oh!

Uh oh!

utam0k commented Aug 5, 2025

Uh oh!

tommady commented Aug 6, 2025

Uh oh!

YJDoc2 commented Aug 7, 2025

Uh oh!

tommady commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

utam0k commented Aug 10, 2025

Uh oh!

tommady commented Aug 11, 2025

Uh oh!

utam0k commented Aug 11, 2025

Uh oh!

YJDoc2 commented Aug 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tommady commented Jul 20, 2025 •

edited

Loading

tommady commented Aug 7, 2025 •

edited

Loading