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

Heroku deployment configuration #105

Merged
merged 13 commits into from
Mar 15, 2024
Merged

Conversation

mvandenburgh
Copy link
Member

@mvandenburgh mvandenburgh commented Mar 12, 2024

This PR overhauls the current dandidav deployment process. Currently, we have Heroku configured in a "pull" model, i.e. Heroku constantly pulls our repo and reacts to new commits on main by doing a new deployment. This PR changes this to a "push" model, where we instead explicitly build and deploy the application on push to main via the Heroku CLI in a GitHub Action. Note, a side effect of this is we have complete control over the environment since we're running cargo build in the GitHub Action as opposed to on Heroku; thus, the commit hash issue is no longer a problem.

It contains two components:

  • A Procfile for Heroku
    • This defines the command for the web dyno to run in order to start the server
  • The GitHub Action that performs the deployment with the Heroku CLI whenever a PR with a "release" label is merged

Other things that need to be done before merging this PR:

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.01%. Comparing base (0b07aa7) to head (c9f6947).
Report is 2 commits behind head on main.

❗ Current head c9f6947 differs from pull request most recent head 23a3521. Consider uploading reports for the commit 23a3521 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #105   +/-   ##
=======================================
  Coverage   46.01%   46.01%           
=======================================
  Files          24       24           
  Lines        3710     3710           
=======================================
  Hits         1707     1707           
  Misses       2003     2003           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jwodder jwodder left a comment

Choose a reason for hiding this comment

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

I would rather not use auto at all. Instead, deployment should be triggered by any merge of a PR that does not have the "skip deployment" label (See here for a similar trigger definition).

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
run: heroku plugins:install heroku-builds

- name: Create Heroku Build
run: heroku builds:create --app dandidav
Copy link
Member

Choose a reason for hiding this comment

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

Is this deploying the entire repository, including the whole of the target/ directory? Can we not just deploy only the target/release/dandidav asset?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the heroku builds plugin doesn't allow specifying a single asset. And additionally, you have to include the Procfile.

I didn't realize how many files got stored in the target/ directory until I just ran the compilation locally, and I agree it's not ideal to push all those artifacts to Heroku. I tried getting around it by storing things in a temporary directory and pushing that - 6b8df2c

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
@jwodder
Copy link
Member

jwodder commented Mar 12, 2024

@mvandenburgh

Set the HEROKU_API_KEY and HEROKU_EMAIL secrets on the dandi/dandidav repo

Set them to what values?

@jwodder jwodder added CI Continuous integration / continuous deployment setup deployment labels Mar 12, 2024
@jwodder
Copy link
Member

jwodder commented Mar 13, 2024

@mvandenburgh Also, please enable caching of dependencies etc. like is done in test.yml.

mvandenburgh and others added 6 commits March 13, 2024 16:27
Co-authored-by: John T. Wodder II <[email protected]>
Co-authored-by: John T. Wodder II <[email protected]>
Instead, deployment is triggered by any merge of a PR that does not
have the "skip deployment" label.
Limits the files pushed to Heroku to the `Procfile` and the `dandidav`
binary. Some extra steps are done in the GitHub Action because the
`heroku builds:create` command only allows specifying a directory,
not an individual file.
@mvandenburgh
Copy link
Member Author

@mvandenburgh

Set the HEROKU_API_KEY and HEROKU_EMAIL secrets on the dandi/dandidav repo

Set them to what values?

I thought we needed someone with admin permissions to set those, but it appears my credentials suffice. So this is all set now

@mvandenburgh
Copy link
Member Author

@jwodder I believe I addressed all your feedback, PTAL and let me know if you have any further questions and/or change requests.

@mvandenburgh mvandenburgh requested a review from jwodder March 14, 2024 00:43
@mvandenburgh
Copy link
Member Author

Also, here is an example of this new workflow running successfully on a different branch - https://github.com/mvandenburgh/dandidav/actions/runs/8273558324/job/22637534296

.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
.github/workflows/deploy.yml Outdated Show resolved Hide resolved
@mvandenburgh mvandenburgh requested a review from jwodder March 15, 2024 00:40
@jwodder jwodder merged commit d1c7edf into dandi:main Mar 15, 2024
8 checks passed
@mvandenburgh mvandenburgh deleted the heroku-deployment branch March 15, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous integration / continuous deployment setup deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants