Allow Dockerfile task to be UP-TO-DATE

[ayedo]

First of all: thanks for your awesome work!

I was wondering: why does the Dockerfile task set the following in the constructor:

Dockerfile() { outputs.upToDateWhen { false } }

I'm using the plugin to start a postgres database during the build in order to generate a jOOQ meta model. Incremental building does not work since in the end, the build chain depends on the Dockerfile task, which is never upToDate.

I tried overwriting the directive in my task, but without success.

[orzeh]

@ayedo I think this is worth considering. I can imagine use cases when generated Dockerfile (task output) does not change for subsequent builds. We could remove the outputs.upToDateWhen { false } statement and allow developers to define task inputs explicit using standard Gradle syntax.

@cdancy WDYT?

@ayedo are you interested in submitting PR?

[cdancy]

@orzeh I agree. Think this was probably left in from when we attempted a go at making docker image reproduction "UP-TO-DATE".

[remmeier]

would be very helpful if this is fixed

[cdancy]

@remmeier submit a PR?

[cdancy]

Removed in master. Next release will have change.

[childnode]

currently we suspect this change to cause images are not rebuild properly with the application plugin (com.bmuschko.docker-java-application) since a dockerDistTar anyway doesn't respect the distTar state and therefor this change in Dockerfile leads to deprecated Dockerfile even the distTar has changed

[kaserf]

Hi, I ran into a problem recently where I would change my "Dockerfile" task (specifically adding some environment variables and changing the entry point), however when I rebuild a task that depends on the "Dockerfile" task, the build task thinks that the "Dockerfile" task is up to date.

Could it be that this is a regression that was introduced by the fix for this issue? It sounds a bit like what @childnode is experiencing as well.

[cdancy]

@kaserf what version of the plugin are you using?

[kaserf]

@cdancy 3.0.9

[cdancy]

@kaserf that's why. Try upgrading to 3.0.12 and let me know how things workout. Fix for this was merged within the past couple releases.

[kaserf]

Just tried with 3.0.12, same problem.

[kaserf]

@cdancy can you point me to a commit that fixed this? Maybe that helps narrowing down my issue.

[orzeh]

Hi, I ran into a problem recently where I would change my "Dockerfile" task (specifically adding some environment variables and changing the entry point), however when I rebuild a task that depends on the "Dockerfile" task, the build task thinks that the "Dockerfile" task is up to date.

@kaserf You have to explicitly set inputs for task of type Dockefile. Try below snippet:

buildscript {
	repositories {
		jcenter()
	}
}

plugins {
	id "com.bmuschko.docker-remote-api" version "3.0.12"
}

ext {
	alpineVersion = "3"
}

task dockerfile(type: com.bmuschko.gradle.docker.tasks.image.Dockerfile) {
	from "alpine:$alpineVersion"
	inputs.property "alpineVersion", alpineVersion
}

task build {
	dependsOn dockerfile
}

[kaserf]

Thanks, I think there is a slight misunderstanding though!

What about the following change:

task createContext(type: Copy) {
  // copy the relevant files into it's own directory
}

task createDockerfile(type: Dockerfile, dependsOn: createContext) {
  destFile = file("somePath/Dockerfile")
  from baseImage
  instruction(['ENTRYPOINT', '/bin/bash'].join(' '))
}

task createContext(type: Copy) {
  // copy the relevant files into it's own directory
}

task createDockerfile(type: Dockerfile, dependsOn: createContext) {
  destFile = file("somePath/Dockerfile")
  from baseImage
  instruction(['ENTRYPOINT', 'echo', 'foobar'].join(' ')) // <------- changed this line
}

Should this set the up to date check to false? I would assume so. I am changing the task itself, that should always set it to not be up to date.

[orzeh]

@kaserf you are right. I think this is a bug. In Dockerfile class instructions property should have @Input rather then @Internal annotation. @cdancy WDYT?

[childnode]

so, this inputs.property thingy seems not to be a adequate solution since this will force to have a bunch of input.property setters and keep them in sync with any changes made to the Dockerfile ...
(but I see discussion continued since I began to write this ;=) and we're on track)

I would also suggest to implicit define the input properties on every instruction change. If this is the case when you change @Internal to/add @Input in Dockerfile: fine 👍
But please bear in mind to also add a defined Input like inputs.files distTar.outputs.files in case of the Application Plugin dockerDistTar definition to get an updated Dockerfile anytime the distTar changes ;)

Ps.: Time to reopen this issue or creating a PR for?

[cdancy]

@orzeh @kaserf @childnode so while we could make the instructions data structure as Optional and as an Input I don't think that will help folks along here at all. We have every method possible exposed to allow developers to pass along what they need to without having to touch this map. If anything I think we should make this private so that folks are not confused.

@kaserf from your snippet above were you able to move forward? It wasn't clear after trying the above if you were able to get around your problem.

@childnode if you have something in mind please feel free to send in a PR for review.

[kaserf]

Well, my workaround currently is to append this to my gradle file:

tasks.withType(Dockerfile) {
  outputs.upToDateWhen { false }
}

I took over this code from someone else, the gist seems to be not to use instructions but in my case use the entryPoint directive directly? In that case, I agree that this should probably be made private not to confuse people.

[cdancy]

@kaserf correct. You could either use entrypoint method or as I believe you used above the instruction method.

[childnode]

@kaserf that brings us back to the beginning

@cdancy I don't understand in which cases the instruction list is not filled with the relevant information due to extensions on interfaces?

[cdancy]

@childnode there are 2 separate issues here at play. Yours is slightly different and it might warrant opening up another ISSUE/PR as you say if only to avoid folks chiming in here and confusing the flow of conversation.

[kaserf]

Talking about confusion...

@cdancy are you saying the way I am doing things (modulo the upToDateWhen hack forcing it to false), using instruction should work? That's what I got from your comment (#357 (comment)), but it's not working for me. Will try entrypoint later.

[kaserf]

Using entryPoint doesn't work either. If I change the string I pass to entryPoint, gradle does not consider that enough to set the task to "not up to date". Same behavior for me as when using instruction.

Should I open a bug for this @cdancy ?

[orzeh]

OK, I was hacking with this for a while, and it turns out both issues have the same root. After removing outputs.upToDateWhen { false } from constructor, Dockerfile task once executed is up-to-date as long as resulting file exists. This leads to errors when instructions change either in custom Dockerfile task, as in @kaserf case or in generated by the DockerJavaApplicationPlugin as in @childnode case, respectively.

The problem is that Gradle knows task output, but doesn't know anything about inputs. We can't just add @Input annotation to instructions as I suggested earlier, because property must be Serializable (and it is not). Reading Gradle documentation about task inputs I found this:

You can specify a closure or Callable as the value of the property. In which case, the closure or Callable is executed to determine the actual property value.

This can be used to do the following:

Dockerfile() {
   inputs.property("instructionsCache", { instructions.collect { it.build() }.join('\n') })
}

Gradle will track string with instructions, which is the real task input. The closure will be evaluated in task execution phase, so everything should work as expected.

I've prepared proof-of-concept of this solution and will submit it as a PR. Any feedback welcome!

[childnode]

even I see this might be more related to #433 now, I'll continue here as an answer to @orzeh
And since they both have the same root (thanks for pointing out the differences you saw but I didn't) I don't see what to discuss in two threads?! never mind ...

once executed is up-to-date as long as resulting file exists.

what is predictable since gradle always only checks for incoming changes but not if the outcome would change .. have to, because else it will produce an outcome to then decide "not to run the task"

You can specify a closure or Callable as the value of the property. In which case, the closure or Callable is executed to determine the actual property value.

this ^ and the suggested change

inputs.property("instructionsCache", { instructions.collect { it.build() }.join('\n') })

would lead to longer build times, gradle startups due to the fact on any configuration phase, even the task is called or not, the input.property closure is evaluated and therefor the build() for all instructions are called.
Not only this, this also changes the behaviour of error handling or time of errors beeing presented to the user; because now it will be executed anytime, it will show instructions.build() errors even on every configuration phase instead of only on the task execution phase.

so I suggest to take the more affordable way to make instructions list Serializable. The only nut I see is to handle the closures given, but we can just serialize a hash from it, can't we?

just started to write a regression test ... childnode@1e2690c

[cdancy]

@childnode if there is any room for improvement feel free to add that to your branch and we can review it there.

would lead to longer build times

I think this may be Ok as tasks of this nature should generally be smaller and the process time should be quick enough though if there is a better way then by all means put that into your branch/PR and lets have at it! ;)

[childnode]

ok, seems to #433 fixed -both- two out of three (see more in #434) and has no negative affects. Thanks @cdancy and @orzeh for you affords

P.S: I expect

I tried overwriting the directive in my task, but without success.
is not a use case that someone intended but was an outcome of the try and error workaround of this issue before?
Let me ask a little bit different ... is anyone trying to do s.th. like

task dockerFile(type: Dockerfile) {
    outputs.upToDateWhen { moon.inZenith }
    from "alpine"
    environmentVariable "Foo", "Bar"
}

? If yes, this seems no to work in conjunction with the fix from #433

[cdancy]

Closing as this has been addressed in a few PR's in an attempt to alleviate some (not all) of the issues noted.