Skip to content
This repository was archived by the owner on Oct 24, 2022. It is now read-only.

Add a verifier to check if generated Dockerfiles are up-to-date. #262

Merged
merged 1 commit into from
Nov 19, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 106 additions & 18 deletions src/main/scala/org/allenai/plugins/DockerBuildPlugin.scala
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,20 @@ object DockerBuildPlugin extends AutoPlugin {
"The value to use for WORKDIR when generating your Dockerfile. Defaults to \"/stage\"."
)

////////////////////////////////////////////////////////////////////////////////////////////////
// The following settings affect how the plugin behaves, but don't affect file or image output.
////////////////////////////////////////////////////////////////////////////////////////////////

val verifyDockerfileIsStrict: SettingKey[Boolean] = Def.settingKey[Boolean](
"If true, the `verifyDockerfile` task will raise an error if the current Dockerfile does " +
"not match the generated one. Defaults to true."
)

val verifyDockerfileOnBuild: SettingKey[Boolean] = Def.settingKey[Boolean](
"If true, `dockerBuild` will depend on `verifyDockerfile`, and will not build if the " +
"Dockerfile is not up-to-date. Defaults to false."
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default to false? It seems like this is generally the desired behavior, given how easy it is to fall out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's more correct to have this as a CI check, not a build-time check. I think it'll cause a lot of annoyances if it's happening on all builds.

Having a warning on all builds by default is good, but I was getting lazy and didn't want to add a third option . . .

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. I think the warning would be a nice-to-have, but I wouldn't block this on it. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK! I'm adding, it's fairly easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I take that back - it's easy, but not very useful, since the warning gets swallowed in the rest of the build output.

I think forcing this to error is probably a better solution.

)

////////////////////////////////////////////////////////////////////////////////////////////////
// The following keys are for generating dockerfiles; and staging, building, running, and
// pushing images. These should not be overridden from the defaults unless you know what you're
Expand All @@ -139,6 +153,12 @@ object DockerBuildPlugin extends AutoPlugin {
"`dockerfileLocation`."
)

val verifyDockerfile: TaskKey[Boolean] = Def.taskKey[Boolean](
"Checks if the Dockerfile that would be generated by `generateDockerfile` is the same as " +
"the Dockerfile at `dockerfileLocation`. This will raise an error if " +
"`verifyDockerfileIsStrict` is true, and print a warning otherwise."
)

val dockerDependencyStage: TaskKey[File] = Def.taskKey[File](
"Builds a staged directory under target/docker/dependencies containing project " +
"dependencies. This will include a generated Dockerfile. This returns the staging " +
Expand Down Expand Up @@ -301,12 +321,27 @@ object DockerBuildPlugin extends AutoPlugin {
}
}

//////////////////////////////////////////////////////////////////////////////////////////////////
// Definitions for plugin tasks.
//////////////////////////////////////////////////////////////////////////////////////////////////
/** Task to read the current text of the main Dockerfile below the sigil line. This returns None
* if and only if the file exists, but the sigil line cannot be found, since this is considered
* a potential user error.
*/
lazy val customDockerfileContents: Def.Initialize[Task[Option[String]]] = Def.task {
val dockerfile = dockerfileLocation.value
if (dockerfile.exists) {
val lines = IO.readLines(dockerfile)
val remainder = lines.dropWhile(_ != DOCKERFILE_SIGIL)
if (remainder.nonEmpty) {
Some(remainder.tail.mkString("\n") + "\n")
} else {
None
}
} else {
Some("")
}
}

/** Task to build a dockerfile using the current project's sbt settings to populate it. */
lazy val generateDockerfileDef: Def.Initialize[Task[Unit]] = Def.task {
/** Task to build dockerfile contents using the current project's sbt settings and return it. */
lazy val generatedDockerfileContents: Def.Initialize[Task[String]] = Def.task {
val logger = Keys.streams.value.log

// Create the copy commands.
Expand Down Expand Up @@ -352,7 +387,7 @@ object DockerBuildPlugin extends AutoPlugin {
// Create the text for dockerMainArgs and CMD command.
val dockerMainArgsText = dockerMainArgs.value.map('"' + _ + '"').mkString(", ")

val dockerfileContents = s"""# AUTOGENERATED
s"""# AUTOGENERATED
# Most lines in this file are derived from sbt settings. These settings are printed above the lines
# they affect.
#
Expand Down Expand Up @@ -413,23 +448,52 @@ COPY lib lib
# Do not remove this line unless you want your changes overwritten!
$DOCKERFILE_SIGIL
"""
}

// If there is already a Dockerfile, retain any contents past the sigil.
/** Task to check the current dockerfile contents against the generated contents, returning true
* if they are up-to-date.
*/
lazy val checkDockerfile: Def.Initialize[Task[Boolean]] = Def.task {
val newContents = {
val generatedContents = generatedDockerfileContents.value
val customContents = customDockerfileContents.value.getOrElse("")
generatedContents + customContents
}
val dockerfile = dockerfileLocation.value
val existingContents = if (dockerfile.exists) {
val lines = IO.readLines(dockerfile)
val remainder = lines.dropWhile(_ != DOCKERFILE_SIGIL)
if (remainder.nonEmpty) {
remainder.tail.mkString("\n")
} else {
logger.warn(s"Overwriting Dockerfile at $dockerfile...")
""
}
} else {
val oldContents = if (dockerfile.exists) IO.read(dockerfileLocation.value) else ""

newContents == oldContents
}

//////////////////////////////////////////////////////////////////////////////////////////////////
// Definitions for plugin tasks.
//////////////////////////////////////////////////////////////////////////////////////////////////

/** Task to build a dockerfile using the current project's sbt settings to populate it. */
lazy val generateDockerfileDef: Def.Initialize[Task[Unit]] = Def.task {
val generatedContents = generatedDockerfileContents.value
val customContents = customDockerfileContents.value.getOrElse {
Keys.streams.value.log(s"Overwriting Dockerfile at ${dockerfileLocation.value}...")
""
}
IO.write(dockerfileLocation.value, generatedContents + customContents)
}

IO.write(dockerfile, dockerfileContents + existingContents + "\n")
/** Task verify that the current Dockerfile is up-to-date. Raises an error if
* verifyDockerfileIsStrict is true.
*/
lazy val verifyDockerfileDef: Def.Initialize[Task[Boolean]] = Def.task {
val isUnchanged = checkDockerfile.value
if (!isUnchanged) {
val message = s"Dockerfile ${dockerfileLocation.value} is not up-to-date. Run " +
"the `generateDockerfile` task to fix."
if (verifyDockerfileIsStrict.value) {
sys.error(message)
} else {
Keys.streams.value.log.warn(message)
}
}
isUnchanged
}

/** Task to stage a docker image containing the dependencies of the current project. This is used
Expand Down Expand Up @@ -577,6 +641,27 @@ $DOCKERFILE_SIGIL
dockerDependencyBuild.value
dockerMainStage.value

// Verify the dockerfile.
if (verifyDockerfileOnBuild.value) {
val isUnchanged = checkDockerfile.value
// Note that due to sbt dependency semantics (the fact that all dependencies are always run),
// we duplicate the below logic in the verifyDockerfile task def. Otherwise, we couldn't
// control the error behavior via the verifyDockerfileOnBuild flag above.
if (!isUnchanged) {
if (verifyDockerfileIsStrict.value) {
sys.error(
s"Dockerfile ${dockerfileLocation.value} is not up-to-date, stopping build. Run " +
"`generateDockerfile` to update."
)
} else {
Keys.streams.value.log.warn(
s"Dockerfile ${dockerfileLocation.value} is not up-to-date. Run `generateDockerfile` " +
"to update."
)
}
}
}

val logger = Keys.streams.value.log
logger.info(s"Building main image ${mainImageNameSuffix.value}...")

Expand Down Expand Up @@ -690,7 +775,10 @@ $DOCKERFILE_SIGIL
},
dockerMainArgs := Seq.empty,
dockerWorkdir := "/stage",
verifyDockerfileIsStrict := true,
verifyDockerfileOnBuild := false,
generateDockerfile := generateDockerfileDef.value,
verifyDockerfile := verifyDockerfileDef.value,
dockerDependencyStage := dependencyStageDef.value,
dockerMainStage := mainImageStageDef.value,
dockerDependencyBuild := dependencyBuildDef.value,
Expand Down
2 changes: 2 additions & 0 deletions src/sbt-test/sbt-plugins/simple/docker/build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ appendToDockerfile := {
val dockerfile = dockerfileLocation.value
IO.append(dockerfile, "# TESTING\n")
}

verifyDockerfileOnBuild := true
6 changes: 6 additions & 0 deletions src/sbt-test/sbt-plugins/simple/test
Original file line number Diff line number Diff line change
Expand Up @@ -93,3 +93,9 @@ $ exists docker/target/docker/main/lib/docker.docker-0.1-SNAPSHOT.jar

# Test that we can re-run staging without error.
> docker/dockerMainStage

# Test that changes to the Dockerfile are detected.
> docker/verifyDockerfile
# Trigger a change in the file.
> set dockerPorts.in(docker) += 1234
-> docker/verifyDockerfile