rails-template icon indicating copy to clipboard operation
rails-template copied to clipboard

Fixes for Github Actions

Open joshmcarthur opened this issue 2 years ago • 4 comments

This changeset resolves issues I had with Github Actions not passing on a new Rails application with all the trimmings:

  1. Actions started a DB container, but the new version of database.yml doesn't pick up the env vars to connect to the database. I'm happy to bikeshed what we name the env vars since the current naming scheme is not compatible with the libpq defaults
  2. Actions was running rails db:setup, which is not representative of the complete set of steps that an app might need to run tests. In particular, Sidekiq provisions SIDEKIQ_WEB_USERNAME which is expected to be present. I think the least surprising thing is for actions to run bin/setup to copy environment variables, migrate & seed the database, and anything else the app might consider 'setup'.

joshmcarthur avatar Sep 26 '22 23:09 joshmcarthur

I'm happy to bikeshed what we name the env vars since the current naming scheme is not compatible with the libpq defaults

Maybe I'm missing something, but is it possible for us to use the defaults? That would be my preference.

I think the least surprising thing is for actions to run bin/setup

So this was a conscious decision in the original PR - I think we need to discuss further and decide properly (I personally have not been happy that we're installing overcommit in CI, but that can be addressed in an alternative way if we decide to switch back to running bin/setup).

G-Rath avatar Sep 29 '22 03:09 G-Rath

I created an app with sidekiq & devise and the CI is indeed broken because of ENV vars but I haven't had any issues with the DB env vars

I have a WIP PR at https://github.com/eoinkelly/temp-rails-tpl-github-actions-demo/pull/1/files to get the demo to a working state.

eoinkelly avatar Oct 06 '22 21:10 eoinkelly

@G-Rath We could test for presence of a CI and skip the overcommit stuff in bin/setup.

CI=true seems to have good support:

Of course, that doesn't really address it's potential evolution to included slow steps. I know that was my original point in the referenced issue but upon reflection, we almost never change bin/setup so maybe it's not a big deal?

eoinkelly avatar Oct 06 '22 23:10 eoinkelly

Looked into this. We would need to skip overcommit and seeds. bin/setup also runs db:create:all and then migrate which is a slower than db:setup (which uses db:schema:load instead of db:migrate).

I'm now in favour of sticking with what we currently have.

https://github.com/ackama/rails-template/blob/main/bin/setup#L3-L18

eoinkelly avatar Oct 15 '22 05:10 eoinkelly

The build is actually working now, so merging this PR doesn't really add anything. We've discussed this in Ruby Guild and determined this doesn't really add any value given the build is not broken

joshmcarthur avatar Sep 22 '23 00:09 joshmcarthur