Skip to content

Return error if basename is expanded to blank #37396

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 15, 2018
Merged

Return error if basename is expanded to blank #37396

merged 1 commit into from
Jul 15, 2018

Conversation

yui-knk
Copy link
Contributor

@yui-knk yui-knk commented Jul 5, 2018

Fix: #37325

Signed-off-by: Yuichiro Kaneko spiketeika@gmail.com

@yui-knk yui-knk requested a review from tonistiigi as a code owner July 5, 2018 00:36
boaz0
boaz0 approved these changes Jul 5, 2018
View reviewed changes
Copy link
Member

@boaz0 boaz0 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@thaJeztah
Copy link
Member

Looking good; testing with empty values; both with a :tag and without;

docker build --no-cache -<<'EOF'
ARG SOMETHING
ARG ALPINE

FROM ${SOMETHING} as builder
# No actual build steps, just copying

FROM ${ALPINE}

EXPOSE 8080
EOF
Sending build context to Docker daemon  2.048kB
Step 1/5 : ARG SOMETHING
Step 2/5 : ARG ALPINE
Step 3/5 : FROM ${SOMETHING} as builder
base name (${SOMETHING}) should not be blank

With :tag:

docker build --no-cache -<<'EOF'
ARG SOMETHING
ARG ALPINE

FROM ${SOMETHING}:latest as builder
# No actual build steps, just copying

FROM ${ALPINE}:latest

EXPOSE 8080
EOF
Sending build context to Docker daemon  2.048kB
Step 1/5 : ARG SOMETHING
Step 2/5 : ARG ALPINE
Step 3/5 : FROM ${SOMETHING}:latest as builder
invalid reference format

@thaJeztah
Copy link
Member

Wondering if we already have a test covering the "with :tag" option somewhere

@thaJeztah
Copy link
Member

Also; janky failure is not related; tracked through #32673

10:46:48 FAIL: docker_api_swarm_test.go:296: DockerSwarmSuite.TestAPISwarmLeaderElection
10:46:48 
10:46:48 [dd124c5850d58] waiting for daemon to start
10:46:48 [dd124c5850d58] daemon started
10:46:48 
10:46:48 [df0c66ae7bfc0] waiting for daemon to start
10:46:48 [df0c66ae7bfc0] daemon started
10:46:48 
10:46:48 [d107b37dcc526] waiting for daemon to start
10:46:48 [d107b37dcc526] daemon started
10:46:48 
10:46:48 [dd124c5850d58] exiting daemon
10:46:48 assertion failed: error is not nil: Error response from daemon: rpc error: code = DeadlineExceeded desc = context deadline exceeded
10:46:48 [df0c66ae7bfc0] exiting daemon
10:46:48 [d107b37dcc526] exiting daemon

restarting, but all other tests were green

@codecov
Copy link

codecov bot commented Jul 5, 2018 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #37396   +/-   ##
=========================================
  Coverage          ?   34.99%           
=========================================
  Files             ?      610           
  Lines             ?    44883           
  Branches          ?        0           
=========================================
  Hits              ?    15708           
  Misses            ?    27060           
  Partials          ?     2115

thaJeztah
thaJeztah approved these changes Jul 5, 2018
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

yongtang
yongtang approved these changes Jul 7, 2018
View reviewed changes
Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

I think @tonistiigi had some questions about this; @tonistiigi PTAL

@tonistiigi
Copy link
Member

The real reason why the error didn't appear seems to be that empty string is reused to mean FROM scratch in

moby/daemon/images/image_builder.go

Line 171 in 328c089

if refOrID == "" { // ie FROM scratch
. So unless we want to refactor this to be safer the current solution LGTM but should have a comment about why this extra validation is needed.

@yui-knk
Return error if basename is expanded to blank
c9542d3 
Fix: moby#37325

Signed-off-by: Yuichiro Kaneko <spiketeika@gmail.com>
@yui-knk yui-knk force-pushed the error_when_base_name_resolved_to_blank branch from dc2440c to c9542d3 Compare July 15, 2018 13:29
@yui-knk
Copy link
Contributor Author

yui-knk commented Jul 15, 2018 •
edited
Loading

I added comment. Can you review again when you have a chance?

@yongtang yongtang merged commit 492545e into moby:master Jul 15, 2018
@yui-knk yui-knk deleted the error_when_base_name_resolved_to_blank branch July 15, 2018 22:48
@thaJeztah thaJeztah mentioned this pull request Jul 16, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@boaz0 boaz0 boaz0 approved these changes

@yongtang yongtang yongtang approved these changes

Assignees
No one assigned
Labels
area/builder impact/changelog platform/windows status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yui-knk @thaJeztah @tonistiigi @boaz0 @yongtang @AkihiroSuda @GordonTheTurtle