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

tweaks to work with multi-stage builds and --target #149

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
6 participants
@anlutro
Copy link

commented Aug 14, 2018

This is a naive attempt to solve the problem outlined in https://issues.jenkins-ci.org/browse/JENKINS-44609 but also to allow docker.build('--target foo').

This is really quick work from my part. I would just like some feedback to know that my approach here is valid. I have very little Java experience so would need to set off some time in a weekend or something to get everything fully set up and tested.

I do not think this will work for scenarios where you have a FROM previous-stage, but this is still less broken than before.

@anlutro anlutro force-pushed the alprs:fix-docker-multistage-target branch from 675324b to aeceed5 Aug 14, 2018

fromImage = line.substring(5);
continue;
String[] parts = line.split("\\s+");
// if target is set, we want to find the line which matches "FROM x AS target"

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 15, 2018

You should probably verify that parts[2] == "AS"

This comment has been minimized.

Copy link
@anlutro

anlutro Aug 16, 2018

Author

Maybe... but https://docs.docker.com/engine/reference/builder/#from says there is no other valid format than FROM x or FROM x AS y so if parts[2] is anything other than AS I think Docker will be throwing a fit before Jenkins can parse the file. This is all a dirty hack regardless so I think it's OK to assume this part.

target = parsedArgs[i+1]
}
// determine custom dockerfile
else if ((arg == '-f' || arg.startsWith('--file')) && i < (parsedArgs.length - 1)) {
dockerfile = arg.startsWith('--file=') ? arg.split('=')[1] : parsedArgs[i+1]

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 15, 2018

I'm pretty sure you should be i++ing somewhere in here.

This comment has been minimized.

Copy link
@anlutro

anlutro Aug 16, 2018

Author

The i++ is in the for loop itself.

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 16, 2018

For most of the cases you're looking at a single element of the array and walking forward by one. For -f and --file, you're looking at the next element as well.

Consider: ['-f', '--target=Foo'].
The correct parsing is to treat the second argument as a file. I think your code will treat it both as a file and as an argument indicating target of Foo.

This comment has been minimized.

Copy link
@anlutro

anlutro Aug 16, 2018

Author

Ah, I see what you mean. I didn't write the --file parsing stuff so didn't bother to fix it even if I do think it's buggy, but will give it a second look.

target = parsedArgs[i+1]
}
// determine custom dockerfile
else if ((arg == '-f' || arg.startsWith('--file')) && i < (parsedArgs.length - 1)) {

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 16, 2018

fwiw, this case is wrong for:
parsedArgs: ['--file=foo']
i: 0
arg: --file=foo
0 < 1 - 1

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 16, 2018

You're better served splitting startsWith() before ==:

if (arg.startsWith("--...=")) {
...
} else if (i < (parsedArgs.length - 1) && (arg == ... || arg == ...)) {
...
i++;
}
target = arg.split('=')[1]
}
else if (arg == '--target' && i < (parsedArgs.length - 1)) {
target = parsedArgs[i+1]

This comment has been minimized.

Copy link
@jsoref

jsoref Aug 16, 2018

These two lines could be collapsed if you replaced i+1 with i++ -- but i'm not sure how comfortable people are with doing that...

@jsoref

This comment has been minimized.

Copy link

commented Aug 16, 2018

This looks better, thanks :-)

@chris-str-cst

This comment has been minimized.

Copy link

commented Oct 24, 2018

Is there an estimate, when this will be merged?

@jsoref

This comment has been minimized.

Copy link

commented Oct 24, 2018

For reference, I have no control over that. @abayer and @jglick seem to be the primary people involved...

continue;
String[] parts = line.split("\\s+");
// if target is set, we want to find the line which matches "FROM x AS target"
if (target == null || (parts.length == 4 && parts[3] == target)) {

This comment has been minimized.

Copy link
@AntoineMonnet

AntoineMonnet Nov 12, 2018

the string comparison fails for me ... had to change it to target.equals(parts[3])

@jglick

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

I do not maintain this plugin any more. I always recommend users not use this feature and simply

sh 'docker build -t whatever .'
@Paulomart

This comment has been minimized.

Copy link

commented Dec 13, 2018

Can we get this merged?

@anlutro

This comment has been minimized.

Copy link
Author

commented Dec 13, 2018

If it's an unmaintained plugin I'm going to close the PR. I've switched to sh "docker build ..." anyway. If someone else wants to pick up my unfinished work here in a fork or something, you have my permission.

@anlutro anlutro closed this Dec 13, 2018

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

The simplest approach would be to deprecate dockerFingerprintFrom and the other fingerprinting steps (override isAdvanced and edit help.html) and stop using it from Docker.groovy. There is no maintained code that is actually consuming these fingerprints so it is senseless to spend time fixing bugs in them. Ditto the image fingerprinting API code in docker-commons.

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.