Skip to content

Filter empty lines from "docker ls" response (#52081) #52373

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

Merged

Conversation

williamrandolph
Copy link
Contributor

  • Filter empty lines from docker ls response

In order to cut down on test time, our docker/vagrant tests build the
docker image outside of the vagrant VM. When we get around to launching
the Vagrant VM, we mount that already-built docker image to a known
location. At that point, we need to load the docker image. But we only
want to load it once. As we're running tests, we use "docker ls" to
check whether the local image is loaded for use. Empty output from the
particular ls invocation means no image is loaded.

There was a bug in how we checked this. In Java, splitting an empty
string will yield an array containing one empty string. So when we're
counting the output from the docker ls command, we need to filter out
empty lines in order to proceed to loading the image for docker tests.

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

* Filter empty lines from docker ls response

In order to cut down on test time, our docker/vagrant tests build the
docker image outside of the vagrant VM. When we get around to launching
the Vagrant VM, we mount that already-built docker image to a known
location. At that point, we need to load the docker image. But we only
want to load it once. As we're running tests, we use "docker ls" to
check whether the local image is loaded for use. Empty output from the
particular ls invocation means no image is loaded.

There was a bug in how we checked this. In Java, splitting an empty
string will yield an array containing one empty string. So when we're
counting the output from the docker ls command, we need to filter out
empty lines in order to proceed to loading the image for docker tests.
@williamrandolph williamrandolph added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts backport labels Feb 14, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (:Core/Infra/Packaging)

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM


final long count = Arrays.stream(result.stdout.split("\n"))
.map(String::trim)
.filter(s -> s.isEmpty() == false)
Copy link
Member

Choose a reason for hiding this comment

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

nit: This can be simplified as .filter(not(String::isEmpty))

@williamrandolph
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-matrix-windows

@williamrandolph williamrandolph merged commit 317d146 into elastic:7.6 Feb 14, 2020
@williamrandolph williamrandolph deleted the backport/7.6/docker-ls-bug branch March 6, 2020 14:56
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants