-
Notifications
You must be signed in to change notification settings - Fork 582
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
[Refactor] Move containerd client helper from cmd to pkg/clientutil #1780
Conversation
1c07ff0
to
b1dff00
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.
This snippet is duplicated a lot
namespace, err := cmd.Flags().GetString("namespace")
if err != nil {
return err
}
address, err := cmd.Flags().GetString("address")
if err != nil {
return err
}
client, ctx, cancel, err := clientutil.NewClient(cmd.Context(), namespace, address)
I would suggest that we have a cmd/nerdctl/cmdutil.go
which contains this kind of code if they're very common (e.g., read some flags from cmd and call the corresponding pkg functions). So in this case:
// cmd/nerdctl/cmdutil.go, ignore types....
func newClientFromCmd(cmd) client, ctx, cancel, err {
namespace, err := cmd.Flags().GetString("namespace")
if err != nil {
return err
}
address, err := cmd.Flags().GetString("address")
if err != nil {
return err
}
return clientutil.NewClient(cmd.Context(), namespace, address)
}
Personally, I think it still make sense to have 2 functions (newClientFromCmd, and your clientutil.NewClient) compared to the old newClient which does two jobs.
This shouldn't be a problem. after the #1680 is completed, all the global flagging processes will share a command helper function I want to make the Like @AkihiroSuda has said in #1779 (comment), the code in As I have said above, we will have a globalFlag,err := getGlobalFlag(cmd)
if err!=nil{
return err
}
client, ctx, cancel, err := clientutil.NewClient(cmd.Context(), globalFlag.Address, globalFlag.Namespace) So I think the new wrapper function So IMHO, I think this PR is OK for now. But maybe we can introduce GlobalFlag into this PR? (I'm not sure |
:) Overall the PR LGTM and I agree both the refactor and the PR is necessary. My suggestion is more a nit without much context, which can be seen as "add a 10 line helper in
The helper I suggested is also on the Just repeat, it's a nit and the PR is not blocked on me 😉 thanks for your work! |
38dfd3d
to
2b9347e
Compare
66dc6a3
to
8f58f59
Compare
I prefer to go through the refacto in small steps even if it introduces unnecessary/redundant code, it would make the process of contributing easier. LGTM to retains redundancy code until progressive introduction of |
When I refactor the 'compose log' command, I realized that the 'client helper' should be refactored first. After this PR, can the refactoring of the 'compose xxx' command continue? |
Signed-off-by: Zheao.Li <me@manjusaka.me>
8f58f59
to
6081141
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.
Thanks
Signed-off-by: Zheao.Li me@manjusaka.me