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 Heroku Deployment #15

Merged
merged 8 commits into from
Mar 14, 2023
Merged

Fix Heroku Deployment #15

merged 8 commits into from
Mar 14, 2023

Conversation

Reimirno
Copy link

@Reimirno Reimirno commented Mar 9, 2023

Created a new staging environment in rails (basically the same to the production environment as close as possible, but with email sender error suppressed and database name changed etc.) This is meant for staging in Heroku so it staging environment does not do things that only production app should do (sending email to clients etc).

I ended up didn't changing anything in Procfile because:

The right thing to do (for heroku) is just to:

  • For the first deployment, comment out release command db:prepare and then deploy. Then, run db:schema:load && bin/rails db:seed manually on console.
  • For all subsequent deployment, we uncomment release command db:prepare to allow it to run on subsequent deployments. Not doing so will leads to problems like when we have schema changes (when we add features, for example) that is not applied automatically... then you have to manually run db:migrate or db:prepare (if you have a db setup and schema loaded prepare basically just do migration).

Can we simply replace the release command in Procfile to be db:schema:load && bin/rails db:seed? No. This will fail all subsequent runs because db:schema:load is considered destructive to database and running it a second time will be blocked by rails.

So that is why I ended up uncommenting the original db:prepare. Really, making sure db:prepare works in all cases (eliminating the edge case of first deployment) is the right thing to do, instead of swapping out command. I am not sure why db:prepare failed on the first deployment. Based on logs db:prepare on first deployment seems to fail at the migration stage, complaining either missing version number on migration file or missing table.

@Reimirno Reimirno requested a review from cycomachead March 9, 2023 07:55
@Reimirno
Copy link
Author

Reimirno commented Mar 9, 2023

I will squash these into one commit if you think these are fine since essentially all the commits besides the one creating staging environment are just juggling the procfile and updating readme.

config/cable.yml Outdated
staging:
adapter: redis
url: <%= ENV.fetch("REDIS_URL") { "redis://localhost:6379/1" } %>
channel_prefix: bjc_teacher_tracker_production
Copy link
Member

Choose a reason for hiding this comment

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

This name should probably be updated.

Copy link
Author

Choose a reason for hiding this comment

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

Yea probably.

database: bjc_teachers_prod

production:
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn’t this key be staging?

Copy link
Author

Choose a reason for hiding this comment

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

Oh I think I got prod and stage database name mixed up.

@Reimirno
Copy link
Author

Reimirno commented Mar 9, 2023

Will resolve by today.

Copy link
Member

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

A couple minor notes too

README.md Outdated
;
```
- Using psql
- First run `heroku pg:psql` or `psql bjc_teachers_dev`
Copy link
Member

Choose a reason for hiding this comment

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

You can also just do rails db, locally or on heroku.

Comment on lines 32 to 34
secret_key_base: <%= ENV["SECRET_KEY_BASE"] %>
bjc_password: <%= ENV["BJC_PASSWORD"] %>
piazza_password: <%= ENV["PIAZZA_PASSWORD"] %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
secret_key_base: <%= ENV["SECRET_KEY_BASE"] %>
bjc_password: <%= ENV["BJC_PASSWORD"] %>
piazza_password: <%= ENV["PIAZZA_PASSWORD"] %>
<<: *production

This would reduce some duplication

@Reimirno Reimirno temporarily deployed to sp23-03-bjc-teachers March 10, 2023 08:29 Inactive
@@ -23,7 +23,10 @@ test:

# Do not keep production secrets in the repository,
# instead read values from the environment.
production:
production: &production
Copy link
Author

Choose a reason for hiding this comment

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

<<: *production below will work only if we add this &production alias.

@Reimirno
Copy link
Author

Reimirno commented Mar 10, 2023

Everything mentioned above fixed.

I will make sure to make fewer such careless mistakes in the future.

Tried to deploy this to Heroku and this seem to work good.

@Reimirno Reimirno requested a review from cycomachead March 10, 2023 08:45
Copy link
Member

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

Two super minor things.

config/database.yml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@cycomachead
Copy link
Member

cycomachead commented Mar 13, 2023 via email

Co-authored-by: Michael Ball <[email protected]>
@Reimirno
Copy link
Author

All typo fixed. Merging.

@Reimirno Reimirno merged commit 2168073 into master Mar 14, 2023
@Reimirno Reimirno deleted the heroku-deployment branch March 14, 2023 19:08
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.

2 participants