Run the app locally and on CI using docker compose #213
Conversation
tahb
force-pushed
the
chore/add-docker-compose
branch
30 times, most recently
from
87f515e to
0aeef41
Compare
There was a problem hiding this comment.
The changes here mostly look good, I've left a couple of comment/questions and I think we lost some documentation in the change over.
It was an super interesting journey for me as I am in neither the one script and installing everything or run it all in docker camps. You have opened my eyes to a benefit of having the option of docker for light touch people, perhaps on support, that I had not considered before. 👍
The Rails tempalte always raised lots of interesting questions in my mind and I never know the right place for that discussion, but I am confident it is not here.
This works has been around a while and I would like to help get it merged as I think it is blocking other work you have!
Give me a yell if I can help with anything.
| @@ -15,6 +15,7 @@ services: | |||
| environment: | |||
| DATABASE_URL: postgres://postgres:password@test-db:5432/app-test | |||
| DATABASE_CLEANER_ALLOW_REMOTE_DATABASE_URL: "true" | |||
| AUTOMATICALLY_FIX_LINTING: "true" | |||
There was a problem hiding this comment.
Does this mean everywhere but CI the linters will fix without prompting?
There was a problem hiding this comment.
Yes, when you run script/test it'll attempt to make automatic fixes but on CI it'll cause a failure.
The general approach of automatic fixing is something originally copied from our scripts to rule them all template found here https://github.com/dxw/scripts-to-rule-them-all/blob/main/ruby/rails/script/test. I haven't tried to introduce or change this but have tried to make it work with Docker too.
tahb
force-pushed
the
chore/add-docker-compose
branch
5 times, most recently
from
0f66382 to
312ce81
Compare
|
@mec thanks for your review. I've made a change and/or replied to each comment with what I think 👍 |
There was a problem hiding this comment.
This approach looks sound to me, but I do wonder about the churn in the PR. We introduce a lot of things that we then modify or remove. Could we have reordered the commits and resulted in a cleaner history instead?
There are a few functional issues in this review (marked FIXME) and some suggestions. Don't let me block the merge once you've fixed (or confirmed as not an issue) the functional issues.
tahb
force-pushed
the
chore/add-docker-compose
branch
2 times, most recently
from
0ae78cb to
d6bfed0
Compare
This codifies the knowledge of how to start a set of Docker containers into a reusable Docker Compose format. With Docker Compose CI _and_ local development can use identical environments and work with strong parity.
tahb
force-pushed
the
chore/add-docker-compose
branch
from
d6bfed0 to
cef2d3a
Compare
* We now only call bundle install once, we don't have to copy arguments for `--retry` and `--jobs` across and keep them in sync * We still use the `RAILS_ENV` as the single control for what gem groups would be installed. If running in production, this won't install dev and test. Nothing should change here but the code should be more succinct
[dxw use Brakeman as a static code scanner](https://github.com/dxw/tech-team-rfcs/blob/main/rfc-024-use-brakeman.md). Run this at the end of the order as it takes a while. Usually we'd like quicker rspec feedback when working with known fixing failing tests. Ignore `coverage/` from linting checks in case Brakeman is run without being output to stdout
When Docker Compose builds images it will use the name of the current directory (currently Rails Template) and suffix the name of the container (test). This will create a docker image: rails-template_test That image name is great for the template itself but when it's used as a template a new directory name will be given, for example MyGreatApp. CI uses `docker save` with rails-template_test for caching. If we did nothing then Docker Compose would build against an image of a different name and not be able to take advantage of the inherent advantages of building from cached docker layers. This change removes the needs to add another TODO for the team to change this when using the template by using the Docker Compose project name argument[1] [1] https://docs.docker.com/compose/reference/#use--p-to-specify-a-project-name
* We continue to have a single interface for running common commands: server, console and test. * If we want to use these commands with Docker we call the same script with DOCKER=true set * If we want to use these commands to debug CI we can also do that * Splitting the script structure into multiple files requires us to change the way we call `shellcheck`. If we don't do this, it doesn't test nested files and it errors when given a folder. * Maintain a single place where we define how every environment/build context should test the application in `run_all_tests`. We remove the distinction between CI and not and add in a new variable called `AUTOMATICALLY_FIX_LINTING` which better describe the distinction.
tahb
force-pushed
the
chore/add-docker-compose
branch
from
cef2d3a to
c7923be
Compare
|
@erbridge I've hopefully addressed all the FIXME comments, I added in the switch for |
tahb
force-pushed
the
chore/add-docker-compose
branch
2 times, most recently
from
cf34e02 to
b0f9141
Compare
We still use the original DOCKER variable under the hood as the trigger for how the scripts should operate. The way we set this value has now changed to allow for 2 mechanisms. 1. For regular users we allow a descriptive `PREFER_DOCKER_FOR_DXW_RAILS` to be set in the users environment (whether globally or on a project level). This brings a descriptive name that should not conflict as the very general `DOCKER` might have done. 2. We're also still able to have short hand ways of using these scripts infrequently by using a switch. This was more preferable than writing `DOCKER=true` before each command since users might try to automate this by adding it into their rc files - which may cause those same conflcits we're trying to avoid
`--pattern` is not the conventional way we tell rspec what files to run. As per the documentation [1] we can simply pass files or paths for multiple files in as a parameter. Using the `--pattern` control presents the user with a load file error. ``` $ bundle exec rspec --pattern spec An error occurred while loading ./spec. Failure/Error: return load_without_bootsnap(resolved, wrap) LoadError: cannot load such file -- /Users/x/sites/dxw/rails-template/spec ``` [1] https://rspec.info/documentation/4.0/rspec-rails/#Running_specs
tahb
force-pushed
the
chore/add-docker-compose
branch
from
b0f9141 to
a01bc89
Compare
This change allows developers to run the application locally using Docker if they wish, by introducing Docker Compose. This is at least my preference for working with apps locally and I've found it to be generally viable as part of day-to-day dev.
We have existing scripts and documentation for starting applications without Docker. These remain the same and honour the existing Scripts to Rule Them All pattern.
Using Docker in development has advantages of stronger parity. Given we MUST always use containers to run CI and SHOULD used in live environments using Docker locally can help identify environment shaped bugs earlier in delivery. Containerisation also rules out a whole category of issues related to our local environment, package versions etc. The script/bootstrap does attempt to deal with this but it cannot guarantee a fully compatible environment.
Docker should always be stable given we use it for CI and live environments. Given we have the Docker work, by adding Docker compose we can leverage it for development and simpler getting started instructions (if you just want to have a play around).
Changes in this PR
DOCKER=trueorCI=trueScreenshots of UI changes
Here we can see CI running with our caching work still in full effect (without caching this build takes ~5-7 minutes).