Address review feedback on es default docker image #126330
Conversation
breskeby
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-delivery (Team:Delivery) |
breskeby
added
:Delivery/Packaging
breskeby
force-pushed
the
rework-docker
branch
4 times, most recently
from
9a974b8 to
60b0be6
Compare
| DockerBase(String image, String suffix) { | ||
| this(image, suffix, "apt-get"); | ||
| this(image, suffix, "apt-get", "dockerfile"); | ||
| } |
There was a problem hiding this comment.
I haven't found any usage of this constructor. Is it possible to remove it?
distribution/docker/build.gradle
Outdated
| from(projectDir.resolve("src/docker/${base.dockerfile}")) { | ||
| expand(varExpansions) | ||
| filter SquashNewlinesFilter | ||
| rename ~/Dockerfile\..+$/, 'Dockerfile' |
There was a problem hiding this comment.
I understand that these files should by convention be named Dockerfile.*. However, there are no limitations in what will be set as DockerBase.dockerfile, so maybe it would be safer to do rename like this?
rename base.dockerfile, 'Dockerfile`| if (details.name.equals("Dockerfile")) { | ||
| filter { String contents -> | ||
| return contents.replaceAll('^RUN *.*artifacts-no-kpi.*$', "COPY $distributionFolderName .") | ||
| .replaceAll('^RUN tar -zxf /tmp/elasticsearch.tar.gz --strip-components=1 &&', "RUN ") |
There was a problem hiding this comment.
Can we avoid this second replaceAll by changing the Dockerfile.default?
There was a problem hiding this comment.
I'm not sure. it would come with the cost of another docker operatino. What we avoid when using a local distro is the unpackaging bit. not sure how to change the docker file to make this efficient to cover both scenarios
There was a problem hiding this comment.
Sorry, I've missed the point here. I can see now that these two replaceAll operations are related.
What I still don't get is what is the condition (and where it is) which decides if this transformation for Dockerfile will be executed.
Could you please point me to it?
This addresses feedback we got for our default image at docker-library/official-images#18692 This also introduces separate docker source files to make maintaining those easier.
required for testing for now
This addresses feedback we got for our default image at docker-library/official-images#18692 This also introduces separate docker source files to make maintaining those easier. We cannot take over all suggested changes as we require certain settings to have our packaging tests pass as expected.
💔 Backport failed
You can use sqren/backport to manually backport by running |
This addresses feedback we got for our default image at docker-library/official-images#18692 This also introduces separate docker source files to make maintaining those easier. We cannot take over all suggested changes as we require certain settings to have our packaging tests pass as expected.
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
This addresses feedback we got for our default image at docker-library/official-images#18692 This also introduces separate docker source files to make maintaining those easier. We cannot take over all suggested changes as we require certain settings to have our packaging tests pass as expected. (cherry picked from commit ca19573) # Conflicts: # build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/DockerBase.java # distribution/docker/src/docker/Dockerfile
* Address review feedback on es default docker image (#126330) This addresses feedback we got for our default image at docker-library/official-images#18692 This also introduces separate docker source files to make maintaining those easier. We cannot take over all suggested changes as we require certain settings to have our packaging tests pass as expected. (cherry picked from commit ca19573) # Conflicts: # build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/DockerBase.java # distribution/docker/src/docker/Dockerfile * Adjust docker fips entrypoint and cmd (#127630) Also extract docker fips configuration in explicit docker file * Fix merge issue
This addresses feedback we got for our default image at docker-library/official-images#18692
This also introduces separate docker source files to make maintaining those easier.