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

Rails smoke test during release phase? #1120

Open
CGA1123 opened this issue Feb 7, 2021 · 3 comments · May be fixed by #1124
Open

Rails smoke test during release phase? #1120

CGA1123 opened this issue Feb 7, 2021 · 3 comments · May be fixed by #1124

Comments

@CGA1123
Copy link

CGA1123 commented Feb 7, 2021

Hi, the buildpack currently does some fingerprinting to notice whether the ruby app being deployed is a rails app by attempting to run rails runner.

If the application is a rails application but for some reason can't be loaded properly (maybe some configuration differences between CI and production) this test will fail but the release will still succeed.

We've adding a rake task to all of our applications as part of the release task in our Procfile that does runs rails runner and if there is a failure returns the non-zero exit code causing the release to fail and therefore does not actually release the applications.

This can catch some awkward issues that can slip through dev and CI and cause apps to deploy but not be able to actually process any requests!

require 'open3'

task final_frontier: :environment do
  _, stderr, status = Open3.capture3("rails runner 'puts Rails.env'")

  raise "Rails app failed to boot:\n\n#{stderr}" unless status.success?
end

Is this something that could be useful to be included in this buildpack?
Maybe only added if there is an explicit ENV var that denotes that the applications is expected to be running rails?

@schneems
Copy link
Contributor

schneems commented Feb 8, 2021

Right now rails runner not being able to execute is treated as "we couldn't get enough information" to know what to do. But I do think it's a good indicator of a problem if the command fails that the app won't be able to boot.

I think rolling this out as default behavior is tricky though. It would be easy to scope it to new apps. We could also have an opt-in and an opt-out flag based on an env var.

I'm generally in favor of adding this as a feature of the buildpack, but rollout of any default behavior will be slow and needs to be deliberate.

@schneems schneems closed this as completed Feb 8, 2021
@schneems schneems reopened this Feb 8, 2021
@CGA1123
Copy link
Author

CGA1123 commented Feb 8, 2021

It would be easy to scope it to new apps.

How would this work 🤔 Is there an easy way to determine whether an application is new or not?

We could also have an opt-in and an opt-out flag based on an env var.

I think this makes a lot of sense, might cause some surprises if this is initially released as globally opt-in. Not sure under which circumstance a test like this could throw false-positives, but I know I would be frustrated if deploys started to fail 'randomly' due to buildpack changes.

Would the idea be that it is initially opt-in and then over time/depending on adoption switch to opt-out?

I'm generally in favor of adding this as a feature of the buildpack, but rollout of any default behavior will be slow and needs to be deliberate.

Yay! 😄 What would be the next steps to move this forward?


Some other questions/thoughts that have come to mind:

Is there already an established convention for opting in/out of features on this buildpack? (I guess this is also implicitly asking: what is a good name for this feature? 😄)

What is a good 'test' to establish that an application will boot successfully? (Is running rails runner enough/are there other more suitable alternative?) Likely this test should be somewhat conservative -- i.e. prefer a less complete solution that avoids false positives over a more complete solution that could lead to unnecessarily failing an applications release.

Should there be scope for application owners to modify/customise that test? If so, how? Perhaps by redefining a rake task (release:healthy)? This could also open up the general idea of this feature to be not only rails specific (although likely should have some sensible rails-specific defaults).

@schneems
Copy link
Contributor

schneems commented Feb 8, 2021

How would this work 🤔 Is there an easy way to determine whether an application is new or not?

We've got that baked in. Check out new_app? in the language_pack/ruby.rb file

Would the idea be that it is initially opt-in and then over time/depending on adoption switch to opt-out?

Exactly. We would start with new apps only and letting people opt-in. Then if it goes okay. I can wire the buildpack up to give me metrics of how often this tasks fails, to give me an idea about the potential impact of turning it on by default.

what is a good name for this feature

Usually env vars that start with HEROKU_ are considered "reserved" by us. So HEROKU_RUBY_BOOTCHECK_ENABLE=1 and HEROKU_RUBY_BOOTCHECK_DISABLE=1 are good places to start.

Should there be scope for application owners to modify/customize that test?

People can already provide their own release phase, this is in addition to that.

CGA1123 added a commit to CGA1123/heroku-buildpack-ruby that referenced this issue Feb 9, 2021
Adds "RailsBootcheck" to the Rails buildpack.

RailsBootcheck attempts to load your rails application and will fail the
the build of your application if it is unable to load.

This can help prevent scenarios where you can accidentally ship a
version of your application that cannot boot properly. Which will lead
to errors as the application is released and traffic is routed to it.

You can opt-in to this feature via the `HEROKU_RAILS_BOOTCHECK_ENABLE`
variable, new rails applications will have this variable set by default.

You can opt-out of this feature via the `HEROKU_RAILS_BOOTCHECK_DISABLE`
variable.

Closes heroku#1120
@CGA1123 CGA1123 linked a pull request Feb 9, 2021 that will close this issue
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 a pull request may close this issue.

2 participants