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

Add bootchecking for rails applications #1124

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CGA1123
Copy link

@CGA1123 CGA1123 commented 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 #1120

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 requested a review from a team as a code owner February 9, 2021 00:32
@CGA1123
Copy link
Author

CGA1123 commented Feb 9, 2021

I wanted to open this PR to see whether this is going in the right direction so far.

It's a bit late already for me to look into getting hatchet running today, but I'll make some time tomorrow to do so and get some coverage on these changes.

Thanks 😄

@schneems
Copy link
Contributor

Hey, thanks for the PR! I'm kind of a one-person show right now and feature development on the buildpack tends to be very slow as stability is usually the main driver. There's also other issues (as you've noticed) like our test suite doesn't run for PRs due to sensitive creds and running the suite locally takes a LOOONG time (and can't even execute all the tests).

(Also I just merged in something pretty ambitious around JVM changes. I need to roll that out slowly and carefully).

I appreciate the PR, though. I'm not sure when I'll be able to get around to focusing on this and rolling it out. I think that I would re-use the RailsRunner logic and add a step at the end of def compile and def build to do the check and the logic.

I like that the logic is wrapped up in it's own class that can be tested independently. I'm also in the process of a re-write over at https://github.com/heroku/heroku-buildpack-ruby-experimental and that's the dominant pattern I'm using.

I do want to set the expectation that most larger features like this are not merged wholesale which is unfortunate but also the nature of buildpack development as anything that goes in needs to be supported (quite literally, via customer support tickets) for years to come.

So if you're interested in tweaking this based on that feedback, then I would say go for it. But keep in mind that I might not be able to give you movement or even propper feedback and attention for some long cycles. When I'm ready to revisit this I might be able to pull your change in and work on-top of it, but likely we'll not be able to get to a place where I could just merge and ship the whole thing.

@CGA1123
Copy link
Author

CGA1123 commented Feb 14, 2021

That makes sense. I appreciate the requirements for the buildpacks are different than in most projects and that rolling out is slow and gradual! People want stable buildpacks, companies and developers rely on that (including me, so thank you for keeping the bar high 😄)!

I'm happy to tweak this changeset to make use of the RailsRunner, adding an additional check on success? which raises if this feature is enabled. And hopefully getting this to a place where it is as low friction for you as possible when/if an opportunity comes up to work on this.

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

(Cancelling review to get this out of the languages team review queue, as it's already had a review from the Ruby SME: #1124 (comment))

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.

Rails smoke test during release phase?
3 participants