Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: fix part of megacheck issue #2500

Merged
merged 1 commit into from
Dec 4, 2018
Merged

refactor: fix part of megacheck issue #2500

merged 1 commit into from
Dec 4, 2018

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented Nov 26, 2018

Signed-off-by: zhangyue zy675793960@yeah.net

Ⅰ. Describe what this PR did

As the title desc, but just part of this issue.

Ⅱ. Does this pull request fix one issue?

ref #2495

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

just refactor, no need.

Ⅳ. Describe how to verify it

wait for CI pass.

Ⅴ. Special notes for reviews

leave some of issue, and I add more comment in the issue #2495

@codecov
Copy link

codecov bot commented Nov 26, 2018 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2500   +/-   ##
=========================================
  Coverage          ?   69.19%           
=========================================
  Files             ?      278           
  Lines             ?    18439           
  Branches          ?        0           
=========================================
  Hits              ?    12758           
  Misses            ?     4235           
  Partials          ?     1446
Flag Coverage Δ
#criv1alpha1test 31.21% <27.27%> (?)
#criv1alpha2test 35.39% <27.27%> (?)
#integrationtest 40.57% <51.51%> (?)
#nodee2etest 32.79% <27.27%> (?)
#unittest 26.77% <46.66%> (?)
Impacted Files Coverage Δ
daemon/mgr/container.go 58.69% <ø> (ø)
pkg/user/user.go 40% <ø> (ø)
cri/stream/portforward/portforward.go 40% <ø> (ø)
daemon/mgr/container_logger.go 83.33% <ø> (ø)
registry/auth.go 55.14% <ø> (ø)
cri/criservice.go 56.62% <ø> (ø)
pkg/utils/utils.go 83.02% <ø> (ø)
cri/stream/portforward/httpstream.go 70.94% <ø> (ø)
ctrd/client.go 69.23% <ø> (ø)
pkg/meta/local.go 62.37% <0%> (ø)
... and 21 more

@ZYecho ZYecho force-pushed the fix-megacheck branch 2 times, most recently from af42b50 to bcc8b03 Compare November 26, 2018 03:25
@HusterWan
Copy link
Contributor

please rebase your pr, thanks

allencloud
allencloud reviewed Nov 28, 2018
View reviewed changes
pkg/user/user.go
@@ -33,17 +33,13 @@ type uidParser struct {
placeholder string
uid int
gid int
finger []string
userdir string
shell string
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please help to check whether these fields are useful in your scope. @Ace-Tang

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code have been changed by @rudyfly once, I think it is why make these filed unused, but the function should be fine

@allencloud allencloud mentioned this pull request Nov 30, 2018
Closed
@allencloud
Copy link
Collaborator

Could you please make this move on ? @ZYecho
I think we need to do the field comment part.

@ZYecho ZYecho force-pushed the fix-megacheck branch 3 times, most recently from 06762b7 to 97e1db7 Compare November 30, 2018 07:39
@ZYecho
Copy link
Contributor Author

ZYecho commented Nov 30, 2018

Could you please make this move on ? @ZYecho
I think we need to do the field comment part.

Sorry for late, update.

@ZYecho ZYecho force-pushed the fix-megacheck branch 2 times, most recently from 70bfc52 to f67c866 Compare December 1, 2018 02:50
@allencloud
Copy link
Collaborator

Although all these megacheck issues are checked via tool, I still wish that we could check if the business logic is OK. @fuweid @starnop

fuweid
fuweid reviewed Dec 3, 2018
View reviewed changes
registry/auth.go Outdated Show resolved Hide resolved
refactor: fix part of megacheck issue
2e9e4b7 
Signed-off-by: zhangyue <zy675793960@yeah.net>
@allencloud
Copy link
Collaborator

LGTM

@pouchrobot pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Dec 4, 2018
@allencloud allencloud merged commit 401e130 into AliyunContainerService:master Dec 4, 2018
@ZYecho ZYecho mentioned this pull request Dec 6, 2018
Merged
@pouchrobot pouchrobot mentioned this pull request Dec 7, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@starnop starnop starnop left review comments

@fuweid fuweid fuweid left review comments

@Letty5411 Letty5411 Letty5411 left review comments

@Ace-Tang Ace-Tang Ace-Tang left review comments

@allencloud allencloud allencloud left review comments

Assignees
No one assigned
Labels
kind/refactor LGTM one maintainer or community participant agrees to merge the pull reuqest. size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ZYecho @HusterWan @allencloud @Ace-Tang @Letty5411 @fuweid @starnop @pouchrobot