fixes #2817 container run with the empty label #2824
Conversation
|
We found this is your first time to contribute to Pouch, @Flsorescence |
Codecov Report
@@ Coverage Diff @@
## master #2824 +/- ##
==========================================
- Coverage 69.19% 69.14% -0.05%
==========================================
Files 279 279
Lines 17607 17603 -4
==========================================
- Hits 12183 12172 -11
Misses 4052 4052
- Partials 1372 1379 +7
|
apis/opts/labels.go
Outdated
| if len(fields) == 1 { | ||
| fields = append(fields, "") | ||
| return fields, nil | ||
| } | ||
| return nil, fmt.Errorf("invalid label %s: label must be in format of key=value", label) |
There was a problem hiding this comment.
If you compare the length of fields with 1, then this line of code is useless. Here is my reason.
Whatever the string label is, when it is been called with fields := strings.SplitN(label, "=", 2), the length of fileds will only be 1 or 2. Then if len(fields) is not 2, then it must be 1. And as a result, code will never run to this line of code return nil, fmt.Errorf("invalid label %s: label must be in format of key=value", label), and it is redudant.
How about doing this to simplify the code and make it more readable:
fields := strings.SplitN(label, "=", 2)
if len(fields) == 1 {
fields = append(fields, "")
}
// length of fields must be 2 when code runs here
return fields, nil
In addition, we can judge whether to define this function parseLabel to only return one parameter []string, you see the error which is returned as the second parameter will always be nil. Thus no need to return it. But it depends on you about this part's modification. @Flsorescence
There was a problem hiding this comment.
Thanks for your comment.
Yes, I know the statement return nil, fmt.Errorf("invalid label %s: label must be in format of key=value", label) will never be executed, but I don't know whether the second parameter is essential for future development.
Thus, I will annotate the useless statement and reserve the second parameter.
There was a problem hiding this comment.
Thanks for the update. I would like to invite @CodeJuan to take a review of this.
apis/opts/labels.go
Outdated
| // Only input key without value | ||
| if len(fields) == 1 { | ||
| fields = append(fields, "") | ||
| return fields, nil |
There was a problem hiding this comment.
duplicate return here. just use global return
BTW, do we need error here?
There was a problem hiding this comment.
I have checked the env command and it has solved the empty value problem.
Do you mean I should modify the code logic of label command as same as env command?
Reserve the parameter of error is beacause I don't know whether it is essential for future development.
I will modify the return statement. Thanks.
Flsorescence
force-pushed
the
dev
branch
2 times, most recently
from
9f6bf62
to
1109525
Compare
Signed-off-by: JoJo <1043708974@qq.com>
|
LGTM |
Ⅰ. Describe what this PR did
fixes the issue #2817 Failed to run a container with empty label
Ⅱ. Does this pull request fix one issue?
fixes #2817
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
I have modified the labels_test.go and add some new unit tests.
Ⅳ. Describe how to verify it
pouch run --label k without value can run successfully
Ⅴ. Special notes for reviews