refactor: mv cli params parse and validate part of code into opts package #1041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HusterWan
force-pushed
the
zr/port-validate
branch
from
7d4877d to
4054dc3
Compare
Codecov Report
@@ Coverage Diff @@
## master #1041 +/- ##
==========================================
+ Coverage 16.03% 16.32% +0.28%
==========================================
Files 142 155 +13
Lines 8685 8770 +85
==========================================
+ Hits 1393 1432 +39
- Misses 7190 7234 +44
- Partials 102 104 +2
|
|
This is a big work. I cannot think of any reason to be against this. While this may bring some influences, we need to pick things carefully. Also related to #1051 for your info. @HusterWan |
allencloud
reviewed
Apr 4, 2018
cli/container.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := opts.VerifyRestartPolicy(restartPolicy); err != nil { |
allencloud
reviewed
Apr 4, 2018
cli/container.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := validateOOMScore(c.oomScoreAdj); err != nil { | ||
| if err := opts.VerifyDiskQuota(diskQuota); err != nil { |
allencloud
reviewed
Apr 4, 2018
cli/container.go
Outdated
|
|
||
| return quotaMaps, nil | ||
| } | ||
|
|
||
| // validateOOMScore validates oom score | ||
| func validateOOMScore(score int64) error { |
There was a problem hiding this comment.
Please move this function to opts as well.
allencloud
reviewed
Apr 4, 2018
daemon/mgr/spec_devices.go
Outdated
| @@ -109,7 +109,7 @@ func setupDevices(ctx context.Context, meta *ContainerMeta, spec *SpecWrapper) e | |||
| } | |||
| } else { | |||
| for _, deviceMapping := range meta.HostConfig.Devices { | |||
| if !runconfig.ValidDeviceMode(deviceMapping.CgroupPermissions) { | |||
| if !opts.ValidDeviceMode(deviceMapping.CgroupPermissions) { | |||
There was a problem hiding this comment.
Actually I do not think validate options in spec is a good choice. Maybe we need to move this part a little bit upper.
Not to block the process of this pull request. Just comment to record it.
allencloud
reviewed
Apr 4, 2018
pkg/opts/devicemappings.go
Outdated
| return results, nil | ||
|
|
||
| } | ||
|
|
||
| // ParseDevice parses a device mapping string to a container.DeviceMapping struct | ||
| func ParseDevice(device string) (*types.DeviceMapping, error) { |
There was a problem hiding this comment.
If this a function which is only used in package opts, I suggest that we make it private with parseDevice.
allencloud
reviewed
Apr 4, 2018
pkg/opts/diskquota.go
Outdated
| } | ||
|
|
||
| // VerifyDiskQuota verifies diskquota configurations of container. | ||
| func VerifyDiskQuota(quotaMaps map[string]string) error { |
allencloud
reviewed
Apr 4, 2018
pkg/opts/networks.go
Outdated
| } | ||
|
|
||
| // VerifyNetworks verifies network configurations of container. | ||
| func VerifyNetworks(nwConfig *types.NetworkingConfig) error { |
allencloud
reviewed
Apr 4, 2018
pkg/opts/portbindings.go
Outdated
| } | ||
|
|
||
| // VerifyPortBinding verify PortMap struct correctness. | ||
| func VerifyPortBinding(portBindings types.PortMap) error { |
allencloud
reviewed
Apr 4, 2018
pkg/opts/portbindings.go
Outdated
| for port := range portBindings { | ||
| _, portStr := nat.SplitProtoPort(string(port)) | ||
| if _, err := nat.ParsePort(portStr); err != nil { | ||
| return fmt.Errorf("invalid port specification: %q", portStr) |
There was a problem hiding this comment.
fmt.Errorf("invalid port specification %q: %s", portStr, %v)
allencloud
reviewed
Apr 4, 2018
pkg/opts/ports.go
Outdated
| } | ||
|
|
||
| // VerifyExposedPorts verify the correction of exposed ports. | ||
| func VerifyExposedPorts(ports map[string]interface{}) error { |
HusterWan
force-pushed
the
zr/port-validate
branch
from
4054dc3 to
4caed31
Compare
HusterWan
changed the title
HusterWan
force-pushed
the
zr/port-validate
branch
from
4caed31 to
639194b
Compare
HusterWan
force-pushed
the
zr/port-validate
branch
from
639194b to
9e15503
Compare
…nd daemon can both use Signed-off-by: Michael Wan <zirenwan@gmail.com>
HusterWan
force-pushed
the
zr/port-validate
branch
from
9e15503 to
6257735
Compare
|
LGTM |
pouchrobot
added
the
LGTM
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Michael Wan zirenwan@gmail.com
Ⅰ. Describe what this PR did
Now pouch daemon still have not params validation, but we need a policy to verify api request params
i refactor cli params validation part to
optspackage, so that we can put all params parse and verify code to it.Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews