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

Change docker-compose defaults, update Docker compose, Dockerfile, tr… #1519

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

murny
Copy link
Contributor

@murny murny commented Mar 4, 2020

…avis, and README accordingly

Big changes:

  • Remove all fedora containers from our docker setups
  • Switched docker-compose.lightweight.yml and docker-compose.yml around. Now docker-compose.yml and docker-compose.development.yml respectfully
  • Removed hacks to run test suite in docker-compose.development.yml (previously docker-compose.yml)
  • Updated our Dockerfile to run again. Now runs Ruby 2.6, Bundler 2.1.4. Includes other minor fixes and updates
  • docker-compose.yml (previously docker-compose.lightweight.yml ) now runs all data stores by default (redis/postgres/solr).
  • Postgres containers now running v11 instead of v9.6
  • Renamed docker-compose.deployment.yml to docker-compose.production.yml
  • TravisCI now only runs solr docker container, instead of full docker-compose.lightweight.yml setup

How to run new simplified docker-compose.yml:

  • Make sure you clear out old and existing volumes. docker-compose down -v
  • If you have postgres/redis running locally, turn them off (e.g: On Linux: sudo service postgresql stop/sudo service redis stop On Mac: brew services stop postgresql/brew services stop redis). As we will now use postgres/redis via docker.
  • Remove export JUPITER_DATABASE_URL=postgresql://jupiter:[email protected]?pool=5 as no longer needed
  • Now just run as normal... docker-compose up -d, prepare db, go to localhost:3000. All should be good to go!

Tested both docker-compose.yml and docker-compose.development.yml all works great locally.

- docker ps

- >
docker run -d -p 8983:8983
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only care about solr docker for travis, so just run this separately (outside of docker-compose)

Copy link
Contributor

Choose a reason for hiding this comment

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

I have very basic knowledge on docker so I would like to take this as a learning opportunity. What do you mean by only caring about solr docker for travis ? I see that we are still referencing the image in docker-compose.

Copy link
Contributor Author

@murny murny Mar 4, 2020

Choose a reason for hiding this comment

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

Previous we were running docker-compose using the docker-compose.lightweight.yml file (which by default only ran fedora/solr).

Here we don't need to run the docker-compose (as we have also changed it, so it would try to setup redis/postgres which would conflict with travis') we only need to run solr docker container. So I am JUST using docker here. No docker-compose at all. I am pulling down this image https://github.com/docker-solr/docker-solr and configuring it myself outside of docker-compose.

@@ -99,31 +96,11 @@ Now everything is ready, you can go and view Jupiter! Just open your favorite br

(Note: ip address may be different if you are using `docker-machine`)

## Want to run the test suite in docker?
Copy link
Contributor Author

@murny murny Mar 4, 2020

Choose a reason for hiding this comment

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

I really don't see much value here with having someone being able to run the test suite in the full development docker setup. The vision I have for the development docker compose is just a quick way for someone to set up Jupiter and be able to run it for demo purposes.

To get the test suite running in a full docker setup is quite tricky and requires a lot of workarounds (complicated hacks around test code, hacks around selenium and hacks around VCR) for very little gain. I doubt anyone actually uses this full docker setup for testing (especially since DockerFile has been broken for a while)? (and if you do I'd push you to use the normal one, as its very easy to install rails/ruby on your local and gives you better control over debugging your test suite).

So as a result, removed all hacks and documentation around running the test suite in the development docker compose setup. This greatly simplifies our docker setup and test suite code. But I am open to opinions if this is something worth putting time in and supporting?

Copy link
Contributor

@mbarnett mbarnett Mar 4, 2020

Choose a reason for hiding this comment

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

Bit of a niggle, but if developers are mostly expected to use docker-compose.yml and docker-compose.development.yml is more for running a full demo environment that isn't usefull for development because you can't easily run tests, should we rename the file to something like docker-compose.demo_env.yml to avoid confusion when onboarding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we could do that, call it docker-compose.demo.yml or something? I'm terrible with naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the name of this and added more info for a new developer to set up their machine to the README (needs more love though 😬)

@@ -5,15 +5,19 @@
# `sudo -u postgres alter user ${USER} superuser
default: &default
adapter: postgresql
encoding: utf8
pool: <%= ENV.fetch("RAILS_MAX_THREADS") { 5 } %>
encoding: unicode
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if i can change this? Unicode is the default for rails. But for some reason we have it set as utf8 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Unicode is just an alias for UTF8 in Postgresql so I think this change would be okay. I ran into an issue where the character set/encoding was different in the rails config than the database in Mysql and Discovery and it wouldn't run migrations correctly. If you look at the history this project was initialized with Mysql and at the time the default was utf8 (now utf8mb4).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I confirmed with matt before merging this. Matt found the same thing that its just an alias 👍 . He also mentioned the same thing about Mysql which it was quite special with locales 😆. Just glad we can make the switch without any issues 😓

@@ -22,13 +26,6 @@ test:
<<: *default
database: jupiter_test

# TODO: Review this later
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed 🎉


# *NOW* we copy the codebase in
COPY . $APP_ROOT

# Add user
RUN addgroup -g 1000 -S app \
Copy link
Contributor Author

@murny murny Mar 4, 2020

Choose a reason for hiding this comment

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

This should fix all files created in full docker-compose being owned by root and all the issues around that (pids being left around, temp files not being able to get cleared, etc etc)

ports:
- "3000:3000"
- '6379:6379'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you change the port?

Copy link
Contributor Author

@murny murny Mar 4, 2020

Choose a reason for hiding this comment

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

Confusing diff. If you look its referencing the redis image. So nothing actually changed (other then I am opening the port on redis as this is now "lightweight" version now instead of full docker dev setup)

…flesh out more info in README for setting up dev environment
mbarnett
mbarnett previously approved these changes Mar 4, 2020
mbarnett
mbarnett previously approved these changes Mar 4, 2020
@murny murny merged commit 63e2b87 into integration_postmigration Mar 4, 2020
@murny murny deleted the refactor-docker branch March 4, 2020 21:43
else
driven_by :selenium_chrome_headless
end
driven_by :selenium, using: :headless_chrome, screen_size: [1400, 1400]
Copy link
Member

Choose a reason for hiding this comment

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

Aw man! I use CAPYBARA_NO_HEADLESS for debugging tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no problems bringing this back! Should I put up a PR for this?

Copy link
Member

Choose a reason for hiding this comment

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


## For deployment (on UAT environment)
## For production deployment (on UAT environment)
Copy link
Member

Choose a reason for hiding this comment

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

Did you deploy to UAT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope! Never did (don't think I can ssh from home either). I tried not to change much for the deployment part. Nothing changed from the docker-compose yml. Only the underlying Dockerfile was updated. So hopefully no breaking changes here

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.

4 participants