bugfix: fix repeated strings reported by goconst by mathspanda · Pull Request #2454 · AliyunContainerService/pouch

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

Signed-off-by: mathspanda [email protected]

Ⅰ. 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

 )

+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

 }

+// 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

+
+// 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


+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 <[email protected]>

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 <[email protected]>

enhance: add goconst check into ci

f6be76f

Signed-off-by: mathspanda <[email protected]>

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

Conversation

mathspanda commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ⅰ. 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

Uh oh!

codecov Bot commented Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mathspanda Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mathspanda Nov 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZYecho Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

ZYecho commented Nov 8, 2018

Uh oh!

starnop Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

mathspanda Nov 8, 2018

Choose a reason for hiding this comment

Uh oh!

allencloud Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

allencloud Nov 9, 2018

Choose a reason for hiding this comment

Uh oh!

allencloud commented Nov 9, 2018

Uh oh!

mathspanda commented Nov 11, 2018

Uh oh!

allencloud commented Nov 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

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