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

Fix build and setup #83

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix build and setup #83

wants to merge 2 commits into from

Conversation

dmitryrck
Copy link
Member

This PR:

  • Fix to the CI
  • Fix the development setup using docker
  • Add setup instructions to README.md using docker
  • Minor changes to the setup instructions when not using docker

I started fixing our build, but then I went down the rabbit hole fixing the docker setup as well. If needed let me know I can split this into two PRs.

@@ -14,8 +14,6 @@ ENV BUNDLE_JOBS 8
ENV APP_HOME /myapp

ARG RAILS_ENV=production
ENV RAILS_ENV=$RAILS_ENV
ENV RACK_ENV=$RAILS_ENV
Copy link
Member Author

Choose a reason for hiding this comment

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

arg is only available during build, but having those two envs here makes them permanent.

This can have an undesired effect in the test environment. In my testings, having those two lines make the test environment use the development database. Which is not a problem because everything runs inside a transaction but it is not ideal.

@parndt, please make sure you have both of them in fly ✌️

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I take it back. I don't think my test environment was using development database because of these two lines.

Yet, I don't think it is a good idea to have the two envs because it makes them permanent.

To run the tests:

docker compose run --rm app bundle exec rspec

Copy link
Member Author

Choose a reason for hiding this comment

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

Appreciate if anyone can review English and if it is easy to understand 🤗.

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.

1 participant