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

Trim container build process. #5730

Merged
merged 12 commits into from May 21, 2019
Merged

Trim container build process. #5730

merged 12 commits into from May 21, 2019

Conversation

icarito
Copy link
Member

@icarito icarito commented May 16, 2019

Fixes #5705, addresses #5683

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updation
  • ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

@@ -31,6 +31,7 @@ install:
- 'if [ "$TRAVIS_PULL_REQUEST" != "false" ]; then gem install danger danger-junit; fi'

script:
- docker-compose exec web bash -c "apt install -y phantomjs"
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we place this installation in our Dockerfile instead of executing this every build run. It would make easier when we accomplish this publiclab/mapknitter#598

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review!

This pulls libqt5webkit5 and hundreds of megabytes of libraries, and is not needed in production. At the moment it is saving some time and storage for building the container in production and making image more manageable for debugging it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway we can revisit when we do start using docker images (exciting!). For now this should help trim testing and simplify deployment to production (where phantomjs isn't needed).

@plotsbot
Copy link
Collaborator

plotsbot commented May 21, 2019

2 Messages
📖 @icarito Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution! And don’t be discouraged if you see errors – we’re here to help.
📖 Pull Request is marked as Work in Progress

Generated by 🚫 Danger

@icarito icarito changed the title [WIP] Trim container build process. Trim container build process. May 21, 2019
@icarito
Copy link
Member Author

icarito commented May 21, 2019

Finally this sucesfully trims a few minutes out of testing by optimizing build time! I think it's ready to merge...

@icarito icarito added the ready label May 21, 2019
@jywarren
Copy link
Member

jywarren commented May 21, 2019 via email

@jywarren jywarren merged commit 794df37 into master May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve overall testing speed
4 participants