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

Include log4j transform jar in docker build context tasks #69808

Merged

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Mar 2, 2021

When updating our Iron Bank image for 7.11.1, I realised that we weren't including the log4j transform jar in the build context. This PR fixes that, along with some Dockerfile tweaks.

Edit: turns out we need this PR to fix #69924.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Just a clarifying question for my own edification.

@@ -11,4 +20,8 @@ tasks.register("buildIronBankDockerBuildContext", Tar) {
// We always treat Iron Bank builds as local, because that is how they
// are built
with dockerBuildContext(Architecture.X64, false, DockerBase.IRON_BANK, true)

into('scripts') {
from configurations.transformLog4jJar
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we including this a second time again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not - in distribution/docker/build.gradle, the jar is copied by the copy*DockerContext task. That task also calls the extension method dockerBuildContext, like the standalone context subprojects do.

The whole setup here is a bit weird. AFAICT, it exists so that we can generate a Docker context without having to do all the rest of the build. However, and again AFAICT, because we share most of the build context logic via an extension method, it can't (cleanly?) access configurations when called from a subproject. At least, when I tried to just move the jar copy to dockerBuildContext, Gradle got angry with me. Suggestions welcome!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think really the "solution" here is to ditch the separate projects just for creating the context and just make that a separate task in the associated project. I'm not completely certain why we went with this pattern to begin with other than creating more projects means better parallel execution but copying contexts isn't particularly expensive and DockerBuild can already be executed concurrently w/ other tasks in the same project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We may be building things these way partly for the release-manager - we need to coordinate there before changing this.

@pugnascotia pugnascotia changed the title Improvements to generating the Iron Bank build context Include log4j transform jar in docker build context tasks Mar 3, 2021
@pugnascotia pugnascotia merged commit 477ded9 into elastic:master Mar 3, 2021
@pugnascotia pugnascotia deleted the iron-bank-docker-improvements branch March 3, 2021 20:24
pugnascotia added a commit that referenced this pull request Mar 3, 2021
Ensure that the log4j transform jar is included in the generated Docker build context.
Also makes some small fixes to the Iron Bank Dockerfile.
pugnascotia added a commit that referenced this pull request Mar 3, 2021
Ensure that the log4j transform jar is included in the generated Docker build context.
Also makes some small fixes to the Iron Bank Dockerfile.
pugnascotia added a commit that referenced this pull request Mar 3, 2021
Ensure that the log4j transform jar is included in the generated Docker build context.
Also makes some small fixes to the Iron Bank Dockerfile.
@pugnascotia
Copy link
Contributor Author

Backports:

This was referenced Mar 15, 2021
easyice pushed a commit to easyice/elasticsearch that referenced this pull request Mar 25, 2021
)

Ensure that the log4j transform jar is included in the generated Docker build context.
Also makes some small fixes to the Iron Bank Dockerfile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker build context tasks fail to include log4j transform jar
4 participants