Skip to content
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

[FIXED JENKINS-44609] Docker multi-stage builds no longer throw errors #111

Merged
merged 3 commits into from Aug 17, 2017

Conversation

Projects
None yet
5 participants
@ericsmalling
Copy link
Member

commented Aug 9, 2017

JENKINS-44609

Instead of grabbing the image name of the first FROM statement in a Dockerfile, now will return the last one.

[FIXED JENKINS-44609] Docker multi-stage builds no longer throw errors
Instead of grabbing the image name of the first FROM statement in a Dockerfile, now will return the last one.
@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2017

Note: the JUnit for this may only work on machines with Docker 17.05 or newer as it tests a multi-stage formatted Dockerfile. If there is a way to detect that, I'll add it to the junit

@@ -325,6 +325,38 @@ private static void grep(File dir, String text, String prefix, Set<String> match
});
}

@Test public void buildWithMultiStage() {

This comment has been minimized.

Copy link
@abayer

abayer Aug 9, 2017

Member

If this is going to break with Docker < 17.05, could we put in an assume like https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/master/pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractDeclarativeTest.java#L218-L227? With the body of that method copied in and the version numbers switched, you should be able to ensure this test only gets run when on Docker 17.05 or later.

" writeFile file: 'child/stuff1', text: 'hello'\n" +
" writeFile file: 'child/stuff2', text: 'world'\n" +
" writeFile file: 'child/stuff3', text: env.BUILD_NUMBER\n" +
" writeFile file: 'child/Dockerfile.other', text: '# This is a test.\\n\\nFROM hello-world AS one\\nARG stuff4=4\\nARG stuff5=5\\nCOPY stuff1 /\\nFROM scratch\\nCOPY --from=one /stuff1 /\\nCOPY stuff2 /\\nCOPY stuff3 /\\n'\n" +

This comment has been minimized.

Copy link
@abayer

abayer Aug 9, 2017

Member

How about using

"  writeFile file: 'child/Dockerfile.other', text: '''# foo\n" +
  "hey, another line\n" +
  "and another\n" +
  "'''\n" +

instead? I don't like looking at really long lines, and within the Pipeline script, you can use the ''' Groovy multiline string stuff.

@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

@abayer - Made changes per your comments, please re-review when you get time.

@abayer

abayer approved these changes Aug 10, 2017

Copy link
Member

left a comment

Looks good - I assume you've done a test run with an earlier docker to make sure it's skipped?

@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

@abayer Yes, tested with Docker 17.03, 17.05 and 17.06 (CE)

@abayer

This comment has been minimized.

Copy link
Member

commented Aug 11, 2017

Cool cool. =)

@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2017

@jglick You seem to be the most prolific committer here - do you have any issue with merging this?

@Multiply

This comment has been minimized.

Copy link

commented Aug 14, 2017

We're stuck with multi-stage builds as well. Hope to get this merged soon. 👍

@ericsmalling ericsmalling changed the title [JENKINS-44609] Docker multi-stage builds no longer throw errors [FIXED JENKINS-44609] Docker multi-stage builds no longer throw errors Aug 15, 2017

@jglick
Copy link
Member

left a comment

I am no longer maintaining this plugin. @abayer might be interested in handling PRs and releases, not sure. Anyway, as usual, you would be better off just running

sh 'docker build whatever-you-like'

and ignoring this plugin feature.

@@ -99,7 +99,7 @@ public String getToolName() {
}
if (line.startsWith("FROM ")) {
fromImage = line.substring(5);
break;
continue;

This comment has been minimized.

Copy link
@jglick

jglick Aug 17, 2017

Member

Arguably would be better to keep a List<String> fromImages and fingerprint them all, just skipping the as … part of the line. (Even better would be to create distinct kinds of facets representing multistage builds.) Anyway there is no maintained code consuming these fingerprints so it matters little.

This comment has been minimized.

Copy link
@ericsmalling

ericsmalling Aug 17, 2017

Author Member

Yeah - that's interesting, but would be an enhancement IMO. I'm considering this just a fix to the error that is happening now.

@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

I understand that sh works, but you're going to have a bunch of bugs flowing in as people move to multi-stage builds. There's already one other person on this thread taking issue about this being an issue for them. Plus, you loose the Groovy build object functionality by dropping to sh, don't you?

@ericsmalling

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

In any case, @abayer - looks like "tag your it" on merging this. :)

@abayer

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

Okiedokie. Merging.

@abayer abayer merged commit b8f7c60 into jenkinsci:master Aug 17, 2017

@deiga

This comment has been minimized.

Copy link

commented Sep 7, 2017

Could we get this released any time soon? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.