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

Refactor Dockerfile, test scripts and CI (WIP) #1101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ambrussimon
Copy link
Contributor

Current progress of backports.

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@@ -1,15 +1,11 @@
.git
*
Copy link
Contributor

Choose a reason for hiding this comment

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

😆

openssl aes-256-cbc -K $encrypted_55750ae1fbc7_key -iv $encrypted_55750ae1fbc7_iv -in .github_deploy_key.enc -out $SSH_KEY_FILE -d;
chmod 600 $SSH_KEY_FILE;
printf "%s\n" \
"Host github.com" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would be a fan of moving these long multi-line bash scripts to a separate file, and invoking them. Keeps the yaml more readable.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mind having the details in here. I like the transparency of a single file.

Copy link
Contributor

@kofalt kofalt left a comment

Choose a reason for hiding this comment

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

Looks significantly cleaner 👏 I like it

Copy link
Member

@gsfr gsfr left a comment

Choose a reason for hiding this comment

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

Nice work!

Can we get rid of the docker folder?


FROM base as testing
ENV MONGO_MAJOR=3.2 \
MONGO_VERSION=3.2.9
Copy link
Member

Choose a reason for hiding this comment

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

MONGO_MAJOR=${MONGO_VERSION%.*}

# /etc/hosts is corrupted if it has lines starting with tab.
# Exit to allow docker to restart.
if grep -P "^\t" /etc/hosts; then
echo "Host mapping in /etc/hosts is buggy, fail contain start."
Copy link
Member

Choose a reason for hiding this comment

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

fail contain*er* start?

@@ -9,4 +9,5 @@ pytest-mock==1.6.0
pytest-watch==3.8.0
pytest==2.8.5
requests_mock==1.3.0
six>=1.11.0
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need six?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing it's here because the GCS storage plugin uses it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can throw a quick comment - lines beginning with # are ignored

@ryansanford
Copy link
Contributor

@gsfr @ambrussimon Let's please not monkey with the docker folder on this pass. There's enough complexity as-is.

I'll be creating corresponding branches in downstream integration projects now.

@ryansanford
Copy link
Contributor

@ambrussimon
A temporary change to .travis.yml to push docker images for this branch would be appreciated, so we can end2end test impact of this PR with our prod CI.

- tests/bin/setup-integration-tests-ubuntu.sh
- DOCKER_DIR="$HOME/.cache/docker"
- secure: HhT1TdJcpqys8juVMw/DIZeK7oD4595TEKH5KlowH7MvwwFAUyQFb5W63F8dgk7elvRG+3fmga/m1JfXO+Iu7PVD912eiNDagW9aB3CEl3Z8zg+JUL8IjpMCkyKQDyJMnfOkrzdxdaqfOK+WmF+13f2qBu9Kc7wdXuzgHQrg4+0= # CI_REGISTRY_USER
- secure: hh7VDZnkxgl/vqHtS4IpXfIAckKpVQvoCzNW7fstr5Mcu8KNiCWIPgObBRm+m13aqpcFTMWQ6lT2kzORz2wWRbDeVhI1eGWOJswGNHPHZLO0Jaei6yfY2nY2mpxZbl+vdg00jkN64mi1ab3e++QgeLFruW0gyNefXX7E5L/mHTs= # CI_REGISTRY_PASS
Copy link
Member

Choose a reason for hiding this comment

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

Should we replace these encrypted variables with Travis env variables, as used in the openssl line below?

- if [ "$TRAVIS_EVENT_TYPE" == "push" -a "$TRAVIS_BRANCH" == "master" ]; then
docker tag core:dist scitran/core:latest;
docker push scitran/core:latest;
fi
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good way to further clean up the conditionals in the after_success block? It's currently rather hard to read, but I do realize there are order dependencies that add complexity. At minimum, let's have consistency in the test order of $TRAVIS_BRANCH and $TRAVIS_EVENT_TYPE.

@gsfr gsfr removed request for nagem and ryansanford May 11, 2018 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants