Skip to content

nydus-image: remove redundant -o short option #550

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

Merged
merged 1 commit into from
Jul 4, 2022
Merged

nydus-image: remove redundant -o short option #550

merged 1 commit into from
Jul 4, 2022

Conversation

changweige
Copy link
Contributor

@changweige changweige commented Jul 4, 2022

Subcommand 'unpack' introduced short option "-o" which is
redundant with global option log-level ending up with a panic

Unfortunately, github CI action didn't catch such regression :-(

INFO [utils - 93:run] - /data00/home/gechangwei/git_repo/nydus-rs/target-fusedev/debug/nydus-image create --bootstrap bootstrap_scratched --blob /data00/home/gechangwei/nydus-test-yard/blobihdgyw05 --log-level info --fs-version 6 --output-json /tmp/tmp46yv4v3_output.json /data00/home/gechangwei/nydus-test-yard/gen_rootfs_scratch
thread 'main' panicked at 'Argument short must be unique

-o is already in use', /home/gechangwei/.cargo/registry/src/mirrors.sjtug.sjtu.edu.cn-4f7dbcce21e258a2/clap-2.34.0/src/app/parser.rs:190:13

stack backtrace:
0: rust_begin_unwind
at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/std/src/panicking.rs:584:5
1: core::panicking::panic_fmt
at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:143:14
2: clap::app::parser::Parser::debug_asserts

Signed-off-by: Changewei Ge [email protected]

Subcommand 'unpack' introduced short option "-p" which is
redundant with global option log-level ending up with a panic

INFO [utils - 93:run] - /data00/home/gechangwei/git_repo/nydus-rs/target-fusedev/debug/nydus-image create --bootstrap bootstrap_scratched  --blob /data00/home/gechangwei/nydus-test-yard/blobihdgyw05 --log-level info --fs-version 6 --output-json /tmp/tmp46yv4v3_output.json /data00/home/gechangwei/nydus-test-yard/gen_rootfs_scratch
thread 'main' panicked at 'Argument short must be unique

	-o is already in use', /home/gechangwei/.cargo/registry/src/mirrors.sjtug.sjtu.edu.cn-4f7dbcce21e258a2/clap-2.34.0/src/app/parser.rs:190:13
stack backtrace:
   0: rust_begin_unwind
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/fe5b13d681f25ee6474be29d748c65adcd91f69e/library/core/src/panicking.rs:143:14
   2: clap::app::parser::Parser::debug_asserts

Signed-off-by: Changewei Ge <[email protected]>
@yqleng1987
Copy link
Contributor

@changweige , a new test job has been submitted. Please wait in patience.

@yqleng1987
Copy link
Contributor

@changweige , The CI test is completed, please check result:

Test CaseTest Result
merge-target-branch✅SUCCESS
build-docker-image✅SUCCESS
compile-nydus✅SUCCESS
compile-ctr-remote✅SUCCESS
compile-nydus-snapshotter✅SUCCESS
start-nydus-snapshotter-config-containerd✅SUCCESS
run-container-with-nydus-image✅SUCCESS

Congratulations, your test job passed!

@hsiangkao
Copy link
Contributor

hsiangkao commented Jul 4, 2022

I'm not sure introducing too many short options without common practice is a good idea in the long term.
My preference is to use limited common short options shared with most other mainstream program, and use long options otherwise all the time

@imeoer imeoer merged commit 2af60b8 into dragonflyoss:master Jul 4, 2022
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.

4 participants