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

Fix loader error with different build syntax #544

Merged
merged 1 commit into from Sep 20, 2017

Conversation

Projects
None yet
6 participants
@vdemeester
Member

vdemeester commented Sep 20, 2017

build: . was not working anymore. Fixing this by adding a new
tranform function for BuildConfig.

/cc @cdrage

Signed-off-by: Vincent Demeester vincent@sbr.pm

Fix loader error with different build syntax
`build: .` was not working anymore. Fixing this by adding a new
tranform function for BuildConfig.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 20, 2017

Codecov Report

Merging #544 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   49.06%   49.07%   +<.01%     
==========================================
  Files         200      200              
  Lines       16438    16447       +9     
==========================================
+ Hits         8066     8072       +6     
- Misses       7952     7955       +3     
  Partials      420      420

codecov-io commented Sep 20, 2017

Codecov Report

Merging #544 into master will increase coverage by <.01%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
+ Coverage   49.06%   49.07%   +<.01%     
==========================================
  Files         200      200              
  Lines       16438    16447       +9     
==========================================
+ Hits         8066     8072       +6     
- Misses       7952     7955       +3     
  Partials      420      420
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 20, 2017

Member

Do we know where it broke, and if this needs to be back ported?

Member

thaJeztah commented Sep 20, 2017

Do we know where it broke, and if this needs to be back ported?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Sep 20, 2017

Member

@thaJeztah from #481. As we don't load it anyway in docker/cli we don't fails there. But using it as a lib, it fails 😓

Member

vdemeester commented Sep 20, 2017

@thaJeztah from #481. As we don't load it anyway in docker/cli we don't fails there. But using it as a lib, it fails 😓

@cdrage

This comment has been minimized.

Show comment
Hide comment
@cdrage

cdrage Sep 20, 2017

Contributor

@vdemeester kinda odd since build is yet to be supported in docker/cli compose, right? or did I miss a recent commit?

Contributor

cdrage commented Sep 20, 2017

@vdemeester kinda odd since build is yet to be supported in docker/cli compose, right? or did I miss a recent commit?

@cdrage

cdrage approved these changes Sep 20, 2017

@thaJeztah

LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Sep 20, 2017

Member

@cdrage it's just that your PR didn't take into account Load and thus didn't take into account that both things below works 👼. I missed that when reviewing the first one 👼

build: .
build:
  context: .
  dockerfile: foo.dockerfile
Member

vdemeester commented Sep 20, 2017

@cdrage it's just that your PR didn't take into account Load and thus didn't take into account that both things below works 👼. I missed that when reviewing the first one 👼

build: .
build:
  context: .
  dockerfile: foo.dockerfile
@cdrage

This comment has been minimized.

Show comment
Hide comment
@cdrage

cdrage Sep 20, 2017

Contributor

@vdemeester Doh. I forgot about that too.

Thanks for the fix 👍

Contributor

cdrage commented Sep 20, 2017

@vdemeester Doh. I forgot about that too.

Thanks for the fix 👍

@dnephin

LGTM

@dnephin dnephin merged commit 09c8f47 into docker:master Sep 20, 2017

9 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 77.77% of diff hit (target 50%)
Details
codecov/project 49.07% (+<.01%) compared to 16804b7
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.10.0 milestone Sep 20, 2017

@vdemeester vdemeester deleted the vdemeester:fix-build-loading branch Sep 21, 2017

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