bugfix: fix repeated strings reported by goconst #2454

mathspanda · 2018-11-08T12:23:47Z

Signed-off-by: mathspanda mathspanda826@gmail.com

Ⅰ. Describe what this PR did

Fix repeated strings reported by goconst.
Add goconst check into ci.

Ⅱ. Does this pull request fix one issue?

#2424

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

None

Ⅳ. Describe how to verify it

gometalinter --disable-all --skip vendor -E goconst ./...

Ⅴ. Special notes for reviews

None

codecov · 2018-11-08T12:23:49Z

Codecov Report

❗ No coverage uploaded for pull request base (master@e7e89c8). Click here to learn what that means.
The diff coverage is 84.78%.

@@            Coverage Diff            @@
##             master    #2454   +/-   ##
=========================================
  Coverage          ?   68.87%           
=========================================
  Files             ?      277           
  Lines             ?    18296           
  Branches          ?        0           
=========================================
  Hits              ?    12602           
  Misses            ?     4260           
  Partials          ?     1434

Flag	Coverage Δ
#criv1alpha1test	`31.47% <17.39%> (?)`
#criv1alpha2test	`35.76% <52.17%> (?)`
#integrationtest	`40.45% <36.95%> (?)`
#nodee2etest	`32.93% <39.13%> (?)`
#unittest	`26.67% <2.17%> (?)`

Impacted Files	Coverage Δ
apis/opts/config/blkio.go	`69.66% <0%> (ø)`
daemon/mgr/system.go	`73.28% <100%> (ø)`
daemon/mgr/spec_mount.go	`84.25% <100%> (ø)`
apis/server/image_bridge.go	`74.1% <100%> (ø)`
apis/server/container_bridge.go	`87.42% <100%> (ø)`
cri/v1alpha2/cri.go	`69.65% <81.81%> (ø)`

ZYecho · 2018-11-08T12:30:50Z

daemon/mgr/system.go

@@ -26,6 +26,12 @@ import (
 	"github.com/sirupsen/logrus"
 )

+const (
+	unknownHostName      = "<unknown>"


Is there is need to add three different const for "unknown", since they're all same ? Just looks weird

Yeap, i have thought about this. Although their value are all same, they represent value of different system info field in unknown scene.

I think it depends on how the three value diverged, it's possible to be different in the future?

It may be hard to confirm.

Yep, I think it's ok here.

ZYecho · 2018-11-08T12:32:16Z

Maybe We can make verify step more accurate in desc, such as gometalinter --enable=goconst ./...

starnop · 2018-11-08T12:35:12Z

cri/metrics/metrics.go

@@ -13,6 +13,23 @@ func init() {
 	Register()
 }

+// Action labels for different pod/image operations
+const (


Thanks for your contribution. In fact, the pouch's HTTP server also needs to collect metrics. So could you please to do the samething for api/servers?

allencloud · 2018-11-09T01:48:21Z

pkg/utils/metrics/actions.go

+
+// Action labels for different pod/container/image operations.
+const (
+	ActionCreate    = "create"


How about changing all this const variables' naming to be XXXActionLabel?
I think this will be a little bit more readable.

allencloud · 2018-11-09T01:50:07Z

daemon/mgr/spec_mount.go

@@ -14,6 +14,21 @@ import (
 	"github.com/sirupsen/logrus"
 )

+const (
+	// PropagationRPrivate represents mount propagation rprivate.
+	PropagationRPrivate = "rprivate"


How about changing all this const variables' naming to be XXXXXPropagationMode?
I think this will be a little bit more readable.

allencloud · 2018-11-09T16:06:32Z

@mathspanda
I think when you finish this pr, we could add goconst in .circleci/config.yml, right?

How about you directly adding -E goconst in https://github.com/alibaba/pouch/blob/master/.circleci/config.yml#L41 ?

gometalinter --disable-all --skip vendor -E gofmt -E goimports -E golint -E ineffassign -E misspell -E vet -E megacheck ./...

Then no one needs to fix config.yml and goconst could serve all the following pull request.

In addition, does the update satisfy you? @starnop

Signed-off-by: mathspanda <mathspanda826@gmail.com>

mathspanda · 2018-11-11T14:43:46Z

How about you directly adding -E goconst in https://github.com/alibaba/pouch/blob/master/.circleci/config.yml#L41 ?
gometalinter --disable-all --skip vendor -E gofmt -E goimports -E golint -E ineffassign -E misspell -E vet -E megacheck ./...
Then no one needs to fix config.yml and goconst could serve all the following pull request.

Done.

allencloud · 2018-11-11T14:54:55Z

That is so great to see this fixed in code and add a defender in circleCI. LGTM

pouchrobot added kind/bug This is bug report for project size/L labels Nov 8, 2018

mathspanda force-pushed the lint-const branch from 7f03c69 to ffb88e4 Compare November 8, 2018 12:28

ZYecho reviewed Nov 8, 2018

View reviewed changes

starnop reviewed Nov 8, 2018

View reviewed changes

mathspanda force-pushed the lint-const branch 3 times, most recently from 2f569c5 to 882e19a Compare November 8, 2018 14:34

allencloud reviewed Nov 9, 2018

View reviewed changes

mathspanda force-pushed the lint-const branch from 882e19a to b16068f Compare November 9, 2018 04:42

mathspanda added 2 commits November 10, 2018 19:30

bugfix: fix repeated strings reported by goconst

8888b24

Signed-off-by: mathspanda <mathspanda826@gmail.com>

enhance: add goconst check into ci

f6be76f

Signed-off-by: mathspanda <mathspanda826@gmail.com>

mathspanda force-pushed the lint-const branch from b16068f to f6be76f Compare November 10, 2018 11:55

mathspanda changed the title ~~bugfix: fix part of repeated strings reported by goconst~~ bugfix: fix repeated strings reported by goconst Nov 10, 2018

allencloud merged commit af17db3 into AliyunContainerService:master Nov 11, 2018

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Nov 11, 2018

pouchrobot mentioned this pull request Nov 16, 2018

WeeklyReport of PouchContainer from 2018-11-09 to 2018-11-16 #2471

Closed

fuweid mentioned this pull request Nov 20, 2018

[linter issues] repeated string in code detected by gometalinter/goconst #2424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: fix repeated strings reported by goconst #2454

bugfix: fix repeated strings reported by goconst #2454

mathspanda commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading

ZYecho Nov 8, 2018

mathspanda Nov 8, 2018 •

edited

Loading

ZYecho Nov 8, 2018

mathspanda Nov 8, 2018 •

edited

Loading

ZYecho Nov 8, 2018

ZYecho commented Nov 8, 2018

starnop Nov 8, 2018

mathspanda Nov 8, 2018

allencloud Nov 9, 2018 •

edited

Loading

allencloud Nov 9, 2018

allencloud commented Nov 9, 2018

mathspanda commented Nov 11, 2018

allencloud commented Nov 11, 2018

bugfix: fix repeated strings reported by goconst #2454

bugfix: fix repeated strings reported by goconst #2454

Conversation

mathspanda commented Nov 8, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Nov 8, 2018 • edited Loading

Codecov Report

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

mathspanda Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

mathspanda Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

ZYecho commented Nov 8, 2018

starnop Nov 8, 2018

Choose a reason for hiding this comment

mathspanda Nov 8, 2018

Choose a reason for hiding this comment

allencloud Nov 9, 2018 • edited Loading

Choose a reason for hiding this comment

allencloud Nov 9, 2018

Choose a reason for hiding this comment

allencloud commented Nov 9, 2018

mathspanda commented Nov 11, 2018

allencloud commented Nov 11, 2018

mathspanda commented Nov 8, 2018 •

edited

Loading

codecov bot commented Nov 8, 2018 •

edited

Loading

mathspanda Nov 8, 2018 •

edited

Loading

mathspanda Nov 8, 2018 •

edited

Loading

allencloud Nov 9, 2018 •

edited

Loading