refactor: make run command args dealing more explicit #1500
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1500 +/- ##
==========================================
+ Coverage 10.64% 10.65% +0.01%
==========================================
Files 84 84
Lines 7588 7609 +21
==========================================
+ Hits 808 811 +3
- Misses 6678 6697 +19
+ Partials 102 101 -1
Continue to review full report at Codecov.
|
allencloud
force-pushed
the
refac-cmd-args
branch
from
99726a2 to
32668c6
Compare
allencloud
force-pushed
the
refac-cmd-args
branch
4 times, most recently
from
e276f35 to
f70ba98
Compare
| ipStr: "10.110.101.1-10.110.101.5", | ||
| wantErr: false, | ||
| }, | ||
| /*{ |
There was a problem hiding this comment.
I am not sure why it would fail if I add this case.
There was a problem hiding this comment.
I am not sure why it would fail if I add this case.
Maybe because 0.x.x.x is not a real ip address
allencloud
force-pushed
the
refac-cmd-args
branch
6 times, most recently
from
c4fa01f to
ecbd521
Compare
apply/run.go
Outdated
| hosts []v2.Host | ||
| func ConstructClusterFromArg(imageName string, runArgs *Args) (*v2.Cluster, error) { | ||
| var passwd string | ||
| if runArgs.Password != "" { |
There was a problem hiding this comment.
Maybe SSH.Passwd can be set directly to runArgs.Password, when runArgs.Password is empty, passwd is also empty.
There was a problem hiding this comment.
Good idea, roger that.
| } | ||
|
|
||
| // 2. validate if it is IP list, like 192.168.0.5,192.168.0.6,192.168.0.7 | ||
| for _, ip := range strings.Split(inputStr, ",") { |
There was a problem hiding this comment.
Do we need to skip empty cases like: x.x.x.x,,x.x.x.y
There was a problem hiding this comment.
I think x.x.x.x,,x.x.x.y is invalid. This would be split into a string array of []string{"x.x.x.x", "","x.x.x.y"}, and net.ParseIP("") will report error.
There was a problem hiding this comment.
We should make the validation as strictly as possible at the very beginning. It could reduce much more complexity. And it could make end-users feel explicit. This time I remove the x.x.x.x:port, since it is quite complicated. We can add this after refactoring if necessary then.
apply/run.go
Outdated
| Port: strconv.Itoa(int(c.runArgs.Port)), | ||
| } | ||
| // validate input nodes IP info | ||
| if err := validateIPStr(runArgs.Nodes); err != nil { |
There was a problem hiding this comment.
When we run a cluster with only master, we get an error because the worker nodes are empty.
There was a problem hiding this comment.
Oh, So good catch. Thanks greatly for your careful review. Updated, now.
utils/net/iputils.go
Outdated
| return nil | ||
| var ips = strings.Split(ipStr, "-") | ||
| if ipStr == "" || !strings.Contains(ipStr, "-") { | ||
| return "", nil |
There was a problem hiding this comment.
The previous method was to change the value of the pointer, now we need to return itself if we don't penetrate the pointer type.
Signed-off-by: Allen Sun <[email protected]>
allencloud
force-pushed
the
refac-cmd-args
branch
from
ecbd521 to
91edad8
Compare
Signed-off-by: Allen Sun <[email protected]>
kakaZhou719
added
optimization
refactor
Signed-off-by: Allen Sun [email protected]
Describe what this PR does / why we need it
Almost make
sealer runcommand argument dealing more explicitly. Here I carry some basic pricinples:defensive programmingto pre-process all the input raw data from command, make all invalid and unformatted data out of the inside main code procedure;int nvalue, just pass thisnumber int, no need to pass the all struct which contains theint nvalue. When data flow is just sufficient, the code readability would be improved a lot. And beginners do not have to learn the struct instance, and just know theinit nvalue meaning.Based on the above principles, this pull request did:
ClusterArgs, which seems to have very short-time lifecycle and may not be necessary;Does this pull request fix one issue?
none
Describe how you did it
none
Describe how to verify it
none
Special notes for reviews
none