-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Remove redundant testing and circular dependency from docker acceptance testing #18181
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
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
This pull request does not have a backport label. Could you fix it @donoghuc? 🙏
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if the env var LOCAL_ARTIFACTS and here could help, so we don't need to do string substitution with sed
and awk
. It is a bit hard to maintain.
If I recall correctly, the env var is used to build docker image with local artifact, but this needs to be verified.
docker/Makefile
Outdated
tar -zxf ../../logstash-$(VERSION_TAG)-docker-build-context.tar.gz && \ | ||
sed 's/artifacts/snapshots/g' Dockerfile > Dockerfile.tmp && mv Dockerfile.tmp Dockerfile && \ | ||
cp ../../logstash-$(VERSION_TAG)-linux-$(ARCHITECTURE).tar.gz . && \ | ||
awk '/# Add Logstash itself and set permissions/{print; print "COPY logstash-$(VERSION_TAG)-linux-$(ARCHITECTURE).tar.gz /tmp/logstash.tar.gz"; next}1' Dockerfile > Dockerfile.tmp && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates a hidden dependency on the precise wording of a comment from the dockerfile template. Is there any way to make this less fragile?
One way would be to add an ERB comment syntax to the dockerfile template that makes it obvious that the line's exact wording is depended upon. As ERB comments, they will not make it through to the generated Dockerfile:
diff --git a/docker/templates/Dockerfile.erb b/docker/templates/Dockerfile.erb index 355a76cff..d92c8bf1a 100644 --- a/docker/templates/Dockerfile.erb +++ b/docker/templates/Dockerfile.erb @@ -64,7 +64,7 @@ RUN \ <% end -%> # Provide a non-root user to run the process -# Add Logstash itself and set permissions +# Add Logstash itself and set permissions<%# HOOK: do not reword %> <% if image_flavor == 'full' || image_flavor == 'oss' -%> RUN groupadd --gid 1000 logstash && \ adduser --uid 1000 --gid 1000 \ @@ -84,9 +84,9 @@ RUN addgroup -g 1000 logstash && \ <% if image_flavor == 'full' || image_flavor == 'oss' -%> arch="$(rpm --query --queryformat='%{ARCH}' rpm)" && \ <% end -%> - curl --fail --location --output logstash.tar.gz <%= url_root %>/<%= tarball %> && \ - tar -zxf logstash.tar.gz -C /usr/share && \ - rm logstash.tar.gz && \ + curl --fail --location --output logstash.tar.gz <%= url_root %>/<%= tarball %> && \<%# HOOK: do not reword %> + tar -zxf logstash.tar.gz -C /usr/share && \<%# HOOK: do not reword %> + rm logstash.tar.gz && \<%# HOOK: do not reword %> mv /usr/share/logstash-<%= elastic_version %> /usr/share/logstash && \ chown -R logstash:root /usr/share/logstash && \ chmod -R g=u /usr/share/logstash && \
Another way would be to pull this logic up to the dockerfile template, and to generate a dockerfile-local
"correctly" (instead of mutating the dockerfile-full
). This seems like the long-term approach, but wiring it all the way through to whatever depends on it may be complex.
Thanks @kaisecheng and @yaauie for the reviews. The reason I did not pursue the local_artifacts route was we hard code those to Line 135 in 2fab5f4
I will track down how to unify our use of the |
I've updated my PR description #18181 (comment) with detailed reasoning. I am open to filing another ticket for explicitly testing public dockerfiles outsie of exhaustive test pipeline, happy to talk through what that might look like :) |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see awk
, sed
, and the redundant make tasks gone! 👍 Highly appreciate your effort in improving CI.
I tested locally for DRA, and it works as expected.
I have small suggestion to clean up the leftover in artifacts.rake
- If
build_docker_from_dockerfiles()
is not used anymore, can we remove it ? - rename
build_docker()
tobuild_docker_from_local_artifacts()
and remove passing the env"LOCAL_ARTIFACTS" => LOCAL_ARTIFACTS
, as it always builds from local
Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline.
Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing.
This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker.
fe862d5
to
a219f32
Compare
Thanks @kaisecheng ! I applied your suggestions (and rebased on head of main). I removed the |
💚 Build Succeeded
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good with the current status. LGTM
@Mergifyio backport 8.18 8.19 9.0 9.1 9.2 |
✅ Backports have been created
|
…ce testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake
…ce testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake
…ce testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake
…ce testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c)
…ce testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake
…ncy from docker acceptance testing (#18253) * Remove redundant testing and circular dependency from docker acceptance testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake * resolve merge conflicts --------- Co-authored-by: Cas Donoghue <[email protected]>
…ency from docker acceptance testing (#18251) * Remove redundant testing and circular dependency from docker acceptance testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake * fix merge conflicts --------- Co-authored-by: Cas Donoghue <[email protected]>
…ency from docker acceptance testing (#18252) * Remove redundant testing and circular dependency from docker acceptance testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake * fix merge conflicts * fix merge conflict and update rake task name --------- Co-authored-by: Cas Donoghue <[email protected]>
…ncy from docker acceptance testing (#18254) * Remove redundant testing and circular dependency from docker acceptance testing (#18181) * Use locally build artifact to build container from public dockerfile Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from `https://snapshots.elastic.co/downloads/logstash'` then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image to `snapshots`. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release. This commit modifies the dockerfile to use the artifact prepared in the first step instead of curling the last published one. This solves both issues as the tests run against the code from the tip fo the branch being tested and there is no dependency on an artifact existing as a result of a unified release pipeline. * Remove redundant docker steps from workflows Previously for docker acceptance tests three steps were performed: 1. Build container images (based on local artifacts) 2. Build "public" dockerfiles 3. Build container based on (a modified) file from step 2. The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency. This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to `false` is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to `true` when testing. * Remove unused function and argument This commit removes the unused function for building from dockerfiles. It also removes an unused argument for the make task for build_docker. (cherry picked from commit a994c7c) # Conflicts: # docker/Makefile # rakelib/artifacts.rake * fix merge conflicts --------- Co-authored-by: Cas Donoghue <[email protected]>
Release notes
[rn:skip]
What does this PR do?
Previously we would build an image (which would not actually be used), build dockerfiles, modify dockerfiles to curl from
https://snapshots.elastic.co/downloads/logstash'
then build the image used for testing based on the modified dockerfile. This resulted in testing the last published image tosnapshots
. This presents two problems 1. The test is running against the last published image (not the tip of the branch being tested) 2. this carries a dependency on both a DRA and unified stack release having been run. Therefor acceptance tests will fail in between the time we bump logstash version and a successful run of unified release.For docker acceptance tests three steps were performed:
The ONLY difference between the dockerfile that ultimately is used to define an image between 1 and 2 is WHERE the logstash source is downloaded from. In acceptance testing we WANT to use the source at the current checkout of logstash (not a remote). Using remote causes a dependency issue when changes are not published. Publishing is tied to unified release and gated on tests so naturally this is a bad fit for that dependency.
This commit removes the redundancy by ONLY generating images for testing (step 1 from above). This also firms up our use of LOCAL_ARTIFACTS. Namely, the ONLY time we want that set to false is when we build a "public" dockerfile. We explicitly set that in the corresponding DRA script now. Similarly we explicitly set it to true when testing.
Why is it important/What is the impact to the user?
n/a
Checklist
How to test this PR locally
You can run the test locally with:
Related issues