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

Add travis test for DockerHub images #49

Closed
wants to merge 7 commits into from
Closed

Add travis test for DockerHub images #49

wants to merge 7 commits into from

Conversation

Tomcli
Copy link
Contributor

@Tomcli Tomcli commented Mar 27, 2018

  • Add a new Travis job to test DockerHub images and modify Travis to run 2 test jobs in parallel.
  • Move the test script commands under etc/travis.
  • Add new helper make function pull-dockerhub-images to pull the DockerHub images with tag 0.0.1-master.
  • Fix Travis should test pulling external images for build #11

@Tomcli Tomcli added the CI/CD label Mar 27, 2018
@Tomcli Tomcli changed the title Add travis test for DockerHub images for non pull request build Add travis test for DockerHub images Mar 27, 2018
@animeshsingh
Copy link

Thanks @Tomcli
Can we give the current images a version number instead of pulling latest?

* Add a new Travis job to test DockerHub images and modify Travis to run 2 test jobs in parallel.
* Move the test script commands under etc/travis.
* Add new helper make function pull-dockerhub-images to pull the DockerHub images with tag 0.0.1-master.
Makefile Outdated
pull-dockerhub-images: $(addprefix pull-, $(TEST_IMAGES))

$(addprefix pull-, $(TEST_IMAGES)): pull-%: %
@TRAVIS_IMAGES=$< make .pull-dockerhub-images
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: perhaps better TRAVIS_IMAGE instead of TRAVIS_IMAGES? (also in lines 221, 226, 229)

Makefile Outdated
@@ -22,6 +22,10 @@ UI_REPO = [email protected]:IBM/FfDL-dashboard.git
CLI_CMD = $(shell pwd)/cli/bin/ffdl-$(UNAME_SHORT)
CLUSTER_NAME ?= mycluster
PUBLIC_IP ?= 127.0.0.1
TRAVIS_IMAGES_VERSION ?= 0.0.1-master
TEST_IMAGES = $(addprefix $(DOCKER_NAMESPACE)/, $(TEST_IMAGES_SUBFIX))
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I think "suffix" would be the more common terminology (TEST_IMAGES_SUFFIX)

@@ -0,0 +1,11 @@
# fail fast
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how we're calling these scripts from .travis.yml, do we need to add shebangs at the top of these scripts?

#!/bin/bash
...

export MAKE_ARGS=--no-print-directory
# pull images from dockerhub
make $MAKE_ARGS pull-dockerhub-images
make $MAKE_ARGS tag-dockerhub-images-to-latest
Copy link
Contributor

@whummer whummer Apr 7, 2018

Choose a reason for hiding this comment

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

Looks like a good workaround solution for now, but I guess at some point we should make the Docker tags parameterizable in the deployment script, so we don't need to overwrite latest tag here (as Animesh mentioned).

Copy link
Contributor

@whummer whummer left a comment

Choose a reason for hiding this comment

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

LGTM

@Tomcli
Copy link
Contributor Author

Tomcli commented Jul 2, 2018

Moved to #111

@Tomcli Tomcli closed this Jul 2, 2018
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.

3 participants