-
Notifications
You must be signed in to change notification settings - Fork 205
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
nydus-image: refactor unpack/compact cli interface #1606
Conversation
As it may break the existing usage of |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1606 +/- ##
==========================================
- Coverage 61.42% 61.28% -0.14%
==========================================
Files 146 146
Lines 48040 48143 +103
Branches 46007 46110 +103
==========================================
- Hits 29510 29506 -4
- Misses 16971 17075 +104
- Partials 1559 1562 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
others LGMT, thanks!
The cleanup of new registry is invalid as TestMain() calls os.Exit() and will not run defer functions. This patch fixes the issue by doing the cleanup explicitly. Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn>
@@ -245,6 +248,43 @@ impl BlobFactory { | |||
} | |||
} | |||
|
|||
pub fn new_backend_from_json( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse new_backend
here? we can also reuse the handle of #[cfg(feature = "backend-xxx")]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for I don't understand how to reuse new_backend as it takes BackendConfigV2 parameter and we need json string here?
Yes. The #[cfg(feature = "backend-xxx")]
macro should be reused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's only add the feature
flag. :)
src/bin/nydus-image/main.rs
Outdated
.required(true), | ||
Arg::new("backend-type") | ||
.long("backend-type") | ||
.help("Type of backend [possible values: localdisk, s3, oss, registry, http-proxy]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe provide a list method of backend types in storage/src/factory.rs
and use it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will try it.
src/bin/nydus-image/main.rs
Outdated
.arg( | ||
Arg::new("backend-type") | ||
.long("backend-type") | ||
.help("Type of backend [possible values: localdisk, s3, oss, registry, http-proxy]") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
others LGTM!
138c3ee
to
33df145
Compare
Since unpack and compact subcommands does not need the entire nydusd configuration file, let's refactor their cli interface and directly take backend configuration file. Specifically, we introduce `--backend-type`, `--backend-config` and `--backend-config-file` options to specify the backend type and remove `--config` option. Signed-off-by: Yifan Zhao <zhaoyifan@sjtu.edu.cn> Fixes: dragonflyoss#1602
33df145
to
4ad1e46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Since unpack and compact subcommands does not need the entire nydusd configuration file, let's refactor their cli interface and directly take backend configuration file.
Specifically, we introduce
--backend-type
,--backend-config
and--backend-config-file
options to specify the backend type and remove--config
option.Fixes: #1602
Relevant Issue (if applicable)
#1602
Details
Introduce
--backend-type
,--backend-config
and--backend-config-file
options forunpack
andcompact
subcommands in nydus-image.Types of changes
Checklist