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.

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

LCOW: Fix nits from 33241 #33826

Merged
merged 1 commit into from Jun 28, 2017

Conversation

Projects
None yet
6 participants
@jhowardmsft
Contributor

jhowardmsft commented Jun 26, 2017

Signed-off-by: John Howard jhoward@microsoft.com

Fixes most of the feedback 'nits' comments from @johnstep in #33241

@johnstep

LGTM

@@ -37,7 +37,7 @@ func (bt *Tagger) TagImages(imageID image.ID) error {
for _, rt := range bt.repoAndTags {
// TODO @jhowardmsft LCOW support. Will need revisiting.
platform := runtime.GOOS
if platform == "windows" && system.LCOWSupported() {
if system.LCOWSupported() {

This comment has been minimized.

@johnstep

johnstep Jun 27, 2017

Contributor

What about the other instances of this redundant (platform == "windows") check?

@johnstep

johnstep Jun 27, 2017

Contributor

What about the other instances of this redundant (platform == "windows") check?

This comment has been minimized.

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

As mentioned on slack, it's a little moot as I'm actively working to remove all these markers anyway. But done. Push imminent.

@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

As mentioned on slack, it's a little moot as I'm actively working to remove all these markers anyway. But done. Push imminent.

@thaJeztah

left one nit, but looks good otherwise

Show outdated Hide outdated builder/dockerfile/parser/parser.go Outdated
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

Comments addressed.

Contributor

jhowardmsft commented Jun 27, 2017

Comments addressed.

@thaJeztah

LGTM

ping @johnstep PTAL

@johnstep

LGTM

@vdemeester

LGTM 👼

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 27, 2017

Contributor

Failures look legit.

Contributor

cpuguy83 commented Jun 27, 2017

Failures look legit.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

@cpuguy83 yup, they do. Investigating

Contributor

jhowardmsft commented Jun 27, 2017

@cpuguy83 yup, they do. Investigating

LCOW: Fix nits from 33241
Signed-off-by: John Howard <jhoward@microsoft.com>
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 27, 2017

Contributor

I see it. Should be fixed in latest push.

Contributor

jhowardmsft commented Jun 27, 2017

I see it. Should be fixed in latest push.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 28, 2017

Contributor

Experimental failure (just locked up and about to timeout) is unrelated. Restarting for the 3rd time.

Contributor

jhowardmsft commented Jun 28, 2017

Experimental failure (just locked up and about to timeout) is unrelated. Restarting for the 3rd time.

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Jun 28, 2017

Contributor

Green 💚 . Finally. (All sorts of CI infrastructure issues on multiple contexts the past few days)

Contributor

jhowardmsft commented Jun 28, 2017

Green 💚 . Finally. (All sorts of CI infrastructure issues on multiple contexts the past few days)

@jhowardmsft jhowardmsft merged commit 950d472 into moby:master Jun 28, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35340 has succeeded
Details
janky Jenkins build Docker-PRs 43941 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4306 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15280 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4028 has succeeded
Details

@jhowardmsft jhowardmsft deleted the Microsoft:jjh/lcownits branch Jun 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment