Skip to content

builder-next: fixes for rootless mode#38806

Merged
cpuguy83 merged 1 commit intomoby:masterfrom
tonistiigi:rootless-build-fixes
Feb 28, 2019
Merged

builder-next: fixes for rootless mode#38806
cpuguy83 merged 1 commit intomoby:masterfrom
tonistiigi:rootless-build-fixes

Conversation

@tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Feb 28, 2019

@AkihiroSuda @tiborvass

Signed-off-by: Tonis Tiigi [email protected]

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Feb 28, 2019

Thanks, LGTM but compilation failing for Windows

@tonistiigi
Copy link
Member Author

tonistiigi commented Feb 28, 2019

@AkihiroSuda updated

@AkihiroSuda
Copy link
Member

AkihiroSuda commented Feb 28, 2019 

08:30:29 daemon/info_unix.go:250:1:warning: exported method Daemon.Rootless should have comment or be unexported (golint)
08:30:30 Build step 'Execute shell' marked build as failure

thaJeztah
thaJeztah reviewed Feb 28, 2019
View reviewed changes
builder/builder-next/controller.go Outdated
Copy link
Member

@thaJeztah thaJeztah Feb 28, 2019

Choose a reason for hiding this comment

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

With more arguments being added, should we just pass opt here? (newExecutor is not exported, so ok to discuss for a follow-up)

Copy link
Member Author

@tonistiigi tonistiigi Feb 28, 2019

Choose a reason for hiding this comment

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

Let's leave this to follow-up

thaJeztah
thaJeztah approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, but left some suggestions, and the failed setting -> failed to set would be nice to have 😅

vdemeester
vdemeester approved these changes Feb 28, 2019
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🕺

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

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

@@            Coverage Diff            @@
##             master   #38806   +/-   ##
=========================================
  Coverage          ?   36.44%           
=========================================
  Files             ?      613           
  Lines             ?    45877           
  Branches          ?        0           
=========================================
  Hits              ?    16721           
  Misses            ?    26860           
  Partials          ?     2296

@cpuguy83
Copy link
Member

cpuguy83 commented Feb 28, 2019 

11:58:12 ----------------------------------------------------------------------
11:58:12 FAIL: docker_cli_run_test.go:1791: DockerSuite.TestRunInteractiveWithRestartPolicy
11:58:12 
11:58:12 assertion failed: 
11:58:12 Command:  /usr/local/cli/docker run -i --name test-inter-restart --restart=always busybox sh
11:58:12 ExitCode: 0
11:58:12 Error:    <nil>
11:58:12 Stdout:   
11:58:12 Stderr:   
11:58:12 
11:58:12 Failures:
11:58:12 ExitCode was 0 expected 11
11:58:13 
11:58:13 ----------------------------------------------------------------------

Doesn't seem related, but does look new to me.

@cpuguy83 cpuguy83 merged commit 45eae4c into moby:master Feb 28, 2019
@thaJeztah thaJeztah mentioned this pull request Apr 18, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Jun 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@vdemeester vdemeester vdemeester approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/builder Build area/rootless Rootless Mode status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tonistiigi @AkihiroSuda @cpuguy83 @vdemeester @thaJeztah @GordonTheTurtle