Skip to content

LCOW: Fix nits from 33241#33826

Merged
lowenna merged 1 commit into
moby:masterfrom
microsoft:jjh/lcownits
Jun 28, 2017
Merged

LCOW: Fix nits from 33241#33826
lowenna merged 1 commit into
moby:masterfrom
microsoft:jjh/lcownits

Conversation

@lowenna

@lowenna lowenna commented Jun 26, 2017

Copy link
Copy Markdown
Member

Signed-off-by: John Howard [email protected]

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

johnstep
johnstep approved these changes Jun 26, 2017
View reviewed changes

@johnstep johnstep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

johnstep
johnstep reviewed Jun 27, 2017
View reviewed changes
Comment thread api/server/backend/build/tag.go Outdated

@johnstep johnstep Jun 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@lowenna lowenna Jun 27, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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
thaJeztah requested changes Jun 27, 2017
View reviewed changes

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

left one nit, but looks good otherwise

Comment thread builder/dockerfile/parser/parser.go Outdated

@thaJeztah thaJeztah Jun 27, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I noticed DefaultPlatformToken is only used in this file; can you un-export it?

@lowenna lowenna Jun 27, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup, fixed. Push coming shortly.

@lowenna

lowenna commented Jun 27, 2017

Copy link
Copy Markdown
Member Author

Comments addressed.

thaJeztah
thaJeztah approved these changes Jun 27, 2017
View reviewed changes

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

ping @johnstep PTAL

johnstep
johnstep approved these changes Jun 27, 2017
View reviewed changes

@johnstep johnstep left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

vdemeester
vdemeester approved these changes Jun 27, 2017
View reviewed changes

@vdemeester vdemeester left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👼

@cpuguy83

cpuguy83 commented Jun 27, 2017

Copy link
Copy Markdown
Member

Failures look legit.

@cpuguy83 cpuguy83 added status/2-code-review status/failing-ci Indicates that the PR in its current state fails the test suite and removed status/4-merge labels Jun 27, 2017
@lowenna

lowenna commented Jun 27, 2017

Copy link
Copy Markdown
Member Author

@cpuguy83 yup, they do. Investigating

LCOW: Fix nits from 33241
4ec9766 
Signed-off-by: John Howard <[email protected]>
@lowenna

lowenna commented Jun 27, 2017

Copy link
Copy Markdown
Member Author

I see it. Should be fixed in latest push.

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

lowenna commented Jun 28, 2017

Copy link
Copy Markdown
Member Author

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

@lowenna

lowenna commented Jun 28, 2017

Copy link
Copy Markdown
Member Author

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

@lowenna
lowenna merged commit 950d472 into moby:master Jun 28, 2017
@lowenna
lowenna deleted the jjh/lcownits branch June 28, 2017 05:56
@thaJeztah thaJeztah added the area/lcow Issues and PR's related to the experimental LCOW feature label Oct 30, 2018
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
+2 more reviewers
@vdemeester vdemeester vdemeester approved these changes
@johnstep johnstep johnstep approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/lcow Issues and PR's related to the experimental LCOW feature status/4-merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lowenna @cpuguy83 @vdemeester @thaJeztah @johnstep @GordonTheTurtle