-
Notifications
You must be signed in to change notification settings - Fork 4k
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 verify stage to TravisCI config #5621
Conversation
@mstruve I guess for this to work we'd need to set up a few env variables, most likely to dummy values. |
Another problem is that
I'll see what else I can come up with, but we may need to move the config from simple lifecycle to actual stages. |
Why not just add it as another step to the initial Hopefully, setting those dummy values aren't a rabbit hole 🤞 |
I was thinking about this, but it just felt like a distinct stage to me (as does storybook updating tbh). But in lieu of stages, this is probably the only sensible approach without completely changing our Travis settings. |
@citizen428 random suggestion: would |
@maestromac It shouldn't be necessary, as the exit code of |
5982870
to
23833bc
Compare
This will try to bring up a production Rails runner to verify that the app can boot.
2314905
to
1e9c69c
Compare
@@ -21,6 +21,14 @@ env: | |||
- RAILS_ENV=test | |||
- CC_TEST_REPORTER_ID=f39e060a8b1a558ebd8aff75d5b9760bf1ae98f3f85d628ae28814f3c66438cd | |||
- DATABASE_URL=postgres://postgres@localhost/ | |||
- APP_PROTOCOL=http:// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstruve I think the amount of dummy env variables needed to get the Rails runner to start up is manageable.
@@ -101,7 +101,7 @@ | |||
|
|||
# DEV uses the RedisCloud Heroku Add-On which comes with the predefined env variable REDISCLOUD_URL | |||
redis_url = ENV["REDISCLOUD_URL"] | |||
redis_url ||= ApplicationConfig["REDIS_URL"] | |||
redis_url ||= ENV["REDIS_URL"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this I always got a uninitialized constant ApplicationConfig (NameError)
on Travis and locally. I think it makes sense to keep this consistent with line 103 anyway.
@mstruve This is ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, one downside is if we remove this from the release-tasks.sh
script it does move us further away from actual production because we are not loading the environment with the actual production ENV variables. If a variable we stub here was misconfigured in production this would pass but production would go down. What do you think about keeping both for now approaches for now?
- SENDGRID_USERNAME_ACCEL=dummy | ||
- SENDGRID_PASSWORD_ACCEL=dummy | ||
- HEROKU_APP_URL=practicaldev.herokuapp.com | ||
- SECRET_KEY_BASE=dummydummydummy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to document that these are env variables only required for the verify stage on Travis. I can see someone looking at this down the road and wondering why we have these or whether it
s safe to change or remove them.
Can we add a comment in the travis.yml
file for our future selves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can and will, was just waiting to see if we even go ahead with this approach at all. 😃 Thanks for the reminder though! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense! And thanks :)
@mstruve Agreed, my intention was not to remove it from
wasn't the most explicit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM , but as @vaidehijoshi mentioned, it'd be good to document those vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me. I'll let @mstruve merge this unless you have last second cold feet on it.
What type of PR is this? (check all applicable)
Description
As discussed on Slack, this PR adds a verify stage to the Travis pipeline which will try to start a production Rails runner to verify that the app can boot up, so we don't solely rely on
deploy-tasks.sh
for that purpose.Related Tickets & Documents
n/a
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
n/a
Added to documentation?