Split pkg/idtools by vieux · Pull Request #33482 · moby/moby

vieux · 2017-06-02T00:30:07Z

Split "github.com/docker/docker/pkg/idtools between github.com/docker/docker/pkg/fsutils1 and github.com/docker/docker/pkg/system`

dnephin · 2017-06-02T02:57:41Z

This is going to conflict heavily with #33362. I hope we can merge that first. It's a more involved change. Sorry I should have put that into the tracking issue.

vieux · 2017-06-02T19:05:27Z

@dnephin ok no worries

thaJeztah · 2017-06-13T11:07:56Z

ping @vieux - #33362 was merged, so this needs an, erm, rebase or rewrite

vieux · 2017-06-16T01:31:20Z

@dnephin rebased.

thaJeztah

LGTM

spotted one typo

thaJeztah · 2017-06-16T09:33:50Z

pkg/system/utils_unix.go

s/synlinks/symlinks/

dnephin · 2017-06-16T18:39:35Z

pkg/system/utils_unix.go

I don't think we should export this two line helper.

I believe with the suggested split up this won't need to be exported anymore, as the only consumers will be in pkg/system/user

@dnephin so you would move the whole utils_unix.go to pkg/system/user ?

The file is only these 2 functions, right? so yes I think it would all move to pkg/system/user.

dnephin · 2017-06-16T18:47:42Z

pkg/fsutils/usergroupadd_linux.go

This is the part of the package that I thought would move to pkg/system. It doesn't have much to do with files.

This file could move to pkg/system/user/usergroupadd_linux.go

dnephin · 2017-06-16T18:56:09Z

pkg/fsutils/usergroupadd_unsupported.go

This file could move to pkg/system/user/....go

dnephin · 2017-06-16T18:57:53Z

pkg/fsutils/idtools_unix.go

This file needs to be split up.

LookupX() would go to pkg/system/user/usergroupadd_linux.go

The file stuff can stay in fsutil

dnephin · 2017-06-16T18:59:15Z

pkg/fsutils/idtools.go

I think some of this file needs to be split up.

All the Mkdir... would be fsutils, but the ID stuff probably makes more sense in system/user/

The file stiff can stay in fsutil

dnephin · 2017-06-16T18:59:57Z

pkg/system/utils_unix.go

I believe with the suggested split up this won't need to be exported anymore, as the only consumers will be in pkg/system/user

Signed-off-by: Victor Vieux <[email protected]>

thaJeztah · 2017-06-24T06:55:45Z

ping @vieux this needs a rebase

vieux · 2017-06-30T15:21:32Z

@thaJeztah yup, @dnephin will carry the PR, assigning to him.

thaJeztah · 2017-07-20T17:45:02Z

ping @dnephin ^^

dnephin · 2017-07-20T18:25:18Z

This seems fairly low priority given the new path for the engine. I'm going to close this PR for now. It's still on my list.

vieux requested a review from dnephin June 2, 2017 00:30

GordonTheTurtle added the status/0-triage label Jun 2, 2017

vieux mentioned this pull request Jun 2, 2017

Find a place for /pkg #32989

Open

79 tasks

AkihiroSuda added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 2, 2017

vieux force-pushed the split_idtools branch from 8cf09a0 to 2cbf0b6 Compare June 2, 2017 00:56

vieux added status/2-code-review and removed status/0-triage status/failing-ci Indicates that the PR in its current state fails the test suite labels Jun 2, 2017

vieux force-pushed the split_idtools branch from 2cbf0b6 to b553cbc Compare June 2, 2017 01:07

AkihiroSuda mentioned this pull request Jun 2, 2017

Increase Coverage of pkg/idtools #33488

Merged

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 13, 2017

vieux force-pushed the split_idtools branch 3 times, most recently from 505cc18 to 4419045 Compare June 16, 2017 01:01

vieux removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 16, 2017

thaJeztah approved these changes Jun 16, 2017

View reviewed changes

pkg/system/utils_unix.go Outdated

Copy link

Member

thaJeztah Jun 16, 2017

Choose a reason for hiding this comment

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

s/synlinks/symlinks/

dnephin suggested changes Jun 16, 2017

View reviewed changes

split idtools

1812bce

Signed-off-by: Victor Vieux <[email protected]>

vieux force-pushed the split_idtools branch from 4419045 to 1812bce Compare June 16, 2017 23:28

GordonTheTurtle assigned vdemeester Jun 17, 2017

vieux assigned dnephin and unassigned vdemeester Jun 30, 2017

dnephin closed this Jul 20, 2017

Comments

Conversation

vieux commented Jun 2, 2017

Uh oh!

dnephin commented Jun 2, 2017

Uh oh!

vieux commented Jun 2, 2017

Uh oh!

thaJeztah commented Jun 13, 2017

Uh oh!

vieux commented Jun 16, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 16, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 24, 2017

Uh oh!

vieux commented Jun 30, 2017

Uh oh!

thaJeztah commented Jul 20, 2017

Uh oh!

dnephin commented Jul 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnephin Jun 16, 2017 •

edited

Loading