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.

Sign up for GitHub

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

Jump to bottom

Refactor the network command flagging process #1822

Merged
merged 1 commit into from
Jan 20, 2023
Merged

Refactor the network command flagging process #1822

merged 1 commit into from
Jan 20, 2023

Conversation

miles170
Copy link
Contributor

See #1680

  • Create a file in pkg/api/types/${cmd}_types.go, and define the CommandOption for this command
  • Create some file in pkg/cmd/${cmd}, and move the command entry point in real into this package

@miles170 miles170 force-pushed the refactor-network branch 2 times, most recently from 1ba00f9 to 71b56ae Compare January 10, 2023 07:14
@miles170
Copy link
Contributor Author

Force-pushed to change the GlobalCommandOptions from pointer to value reference

djdongjin
djdongjin reviewed Jan 14, 2023
View reviewed changes
pkg/api/types/network_types.go Outdated Show resolved Hide resolved
pkg/api/types/network_types.go Outdated Show resolved Hide resolved
cmd/nerdctl/network_create.go Outdated Show resolved Hide resolved
pkg/cmd/network/rm.go Outdated Show resolved Hide resolved
@miles170
Copy link
Contributor Author

miles170 commented Jan 14, 2023 •
edited
Loading

All suggestions have been resolved. thanks❤️

@Zheaoli
Copy link
Member

Zheaoli commented Jan 16, 2023

The code conflict should be removed before merged

@miles170
Copy link
Contributor Author

Force-pushed to resolve code conflicts.

@AkihiroSuda AkihiroSuda mentioned this pull request Jan 17, 2023
Closed
2 tasks
@Zheaoli
Copy link
Member

Zheaoli commented Jan 17, 2023

@miles170 maybe you should resolve conflict again

@miles170
Copy link
Contributor Author

rebased

Zheaoli
Zheaoli approved these changes Jan 17, 2023
View reviewed changes
Copy link
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, on CI green, Thanks

Zheaoli
Zheaoli reviewed Jan 17, 2023
View reviewed changes
cmd/nerdctl/network_create.go
Comment on lines +100 to +113
return network.Create(types.NetworkCreateCommandOptions{
GOptions: globalOptions,
CreateOptions: netutil.CreateOptions{
Name: name,
Driver: driver,
Options: strutil.ConvertKVStringsToMap(opts),
IPAMDriver: ipamDriver,
IPAMOptions: strutil.ConvertKVStringsToMap(ipamOpts),
Subnet: subnetStr,
Gateway: gatewayStr,
IPRange: ipRangeStr,
Labels: labels,
},
}, cmd.OutOrStdout())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: maybe we can keep the same code style with other command like https://github.com/containerd/nerdctl/blob/main/cmd/nerdctl/namespace_create.go#L39-L52(put the option construct process in a single function)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine for now, since most the commandAction simply calls processCommandOption and command.Command (e.g., network.Create) in pkg side.

Later if we decide to move other non-core logic (those more related to cmd than core, e.g., option filtering/postprocess, formatting, outputting, etc) back to cmd side, we can put them in separate sub funcs.

cmd/nerdctl/network_inspect.go
Mode: mode,
Format: format,
Networks: args,
}, cmd.OutOrStdout())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same above

djdongjin
djdongjin requested changes Jan 17, 2023
View reviewed changes
cmd/nerdctl/network_create.go
Comment on lines +100 to +113
return network.Create(types.NetworkCreateCommandOptions{
GOptions: globalOptions,
CreateOptions: netutil.CreateOptions{
Name: name,
Driver: driver,
Options: strutil.ConvertKVStringsToMap(opts),
IPAMDriver: ipamDriver,
IPAMOptions: strutil.ConvertKVStringsToMap(ipamOpts),
Subnet: subnetStr,
Gateway: gatewayStr,
IPRange: ipRangeStr,
Labels: labels,
},
}, cmd.OutOrStdout())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be fine for now, since most the commandAction simply calls processCommandOption and command.Command (e.g., network.Create) in pkg side.

Later if we decide to move other non-core logic (those more related to cmd than core, e.g., option filtering/postprocess, formatting, outputting, etc) back to cmd side, we can put them in separate sub funcs.

cmd/nerdctl/network_prune.go Outdated Show resolved Hide resolved
cmd/nerdctl/system_prune.go Outdated Show resolved Hide resolved
pkg/api/types/network_types.go Show resolved Hide resolved
pkg/cmd/network/prune.go Outdated Show resolved Hide resolved
@miles170
Refactor the network command flagging process
b726a3a 
Signed-off-by: Miles Liu <miles@bung.cc>
@miles170
Copy link
Contributor Author

All suggestions except discussion_r1072302635 have been resolved. thanks❤️

Zheaoli
Zheaoli approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@Zheaoli Zheaoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for now!

@Zheaoli Zheaoli merged commit 20bfde9 into containerd:main Jan 20, 2023
@miles170 miles170 deleted the refactor-network branch January 20, 2023 05:49
@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jan 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djdongjin djdongjin djdongjin requested changes

@Zheaoli Zheaoli Zheaoli approved these changes

Assignees
No one assigned
Labels
kind/refactor
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@miles170 @Zheaoli @djdongjin @AkihiroSuda