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

refactor: use heroku.yml and run migrations in release phase #50

Merged
merged 7 commits into from
Oct 19, 2023

Conversation

wgwz
Copy link
Contributor

@wgwz wgwz commented Sep 28, 2023

Closes: #24

Before deploying this, the following adjustment would need to be made for the production heroku app:

heroku stack:set container --app regen-analytics-staging

The reason that I had to change the heroku deployment method is because graphile-migrate requires a node environment.
The build pack we were using previously was a poetry-python specific build pack, with no easy way to add node support there.

This approach utilizes a version of the dockerfile for the indexer that copied what @ryanchristo put together for the tests.
For a few different reasons, we need a different version of this dockerfile for these changes:

  1. When using the "container" stack (which is heroku's way to let you deploy custom containers), the "build context" for a relative path is always the directory of the dockerfile (this is a heroku feature that can't be changed). So if we tried using the docker/indexer.Dockerfile, when the copy step gets run in heroku, then it would only copy files within the docker/ folder (see heroku's known issues and limitations for containers documented here)

  2. The heroku.yml "run" method was not working with poetry for some reason. It only seemed to be an issue in heroku. When I built and ran the container locally, it was working without issue. So there is some detail about how heroku and "run" command work that was incompatible with how the docker/indexer.Dockerfile works. I had to hack around this by using poetry to export to the "requirements.txt" format and then having the container install the dependencies from there. This is set up in a way where we can still manage our dependencies with poetry and there should be no interruptions otherwise. Just a weird annoying bug.

@wgwz wgwz marked this pull request as ready for review September 29, 2023 00:34
@wgwz wgwz requested a review from a team September 29, 2023 00:34
COPY . .

# Install indexer
RUN pip3 install poetry==1.6.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note: this is an adjustment that i needed to make because previously i was using the python3-poetry from debian package manager, and that version of poetry did not support the export command in line 16 below

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth adding a note to the Install indexer comment about the version and use of export.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

# Install indexer
RUN pip3 install poetry==1.6.1
RUN poetry install
RUN poetry export --without-hashes --format=requirements.txt > requirements.txt
Copy link
Contributor Author

@wgwz wgwz Sep 29, 2023

Choose a reason for hiding this comment

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

this is to work around the weird heroku "run", heroku.yml and poetry bug i mentioned.
this file does not need to be tracked, since it's being generated by poetry.

Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, maybe worth making note of this within the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

release:
image: index-regen
command:
- PGSSLMODE=no-verify yarnpkg migrate
Copy link
Contributor Author

@wgwz wgwz Sep 29, 2023

Choose a reason for hiding this comment

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

PGSSLMODE was needed because without it the underlying node postgres library throws an error.
this tells the pg library under the hood "don't worry about verifying the ssl certificate, but still make use of it"
the node pg library expects you to add certificates to verify ssl connections by default (and that can be tricky).
we do this in the regen-server as well, seems fairly low stakes in this case (this being "use ssl but don't verify")

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

i'd rather wait for #45 to get merged before reviewing this since some of the changes are the same

Base automatically changed from kyle/23-pg-migrate to main October 10, 2023 19:47
@wgwz
Copy link
Contributor Author

wgwz commented Oct 11, 2023

@blushi just a heads up, i merged #45 and rebased this branch, so this is r4r

@wgwz
Copy link
Contributor Author

wgwz commented Oct 11, 2023

i also re-deployed this to the staging analytics environment today:
https://github.com/regen-network/indexer/assets/10120306/6885f3f0-a12d-464c-93a7-ca4ca487f8bb
you can check in with the build logs, release logs and application logs in heroku too.
you'll see the build went well, the release found no new migrations this time and that the script is running as expected (checking out the regular app logs).

@wgwz wgwz requested review from blushi and a team October 16, 2023 15:02
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

LGTM and verified the successful deployment.

i also re-deployed this to the staging analytics environment today

We could set up review apps in heroku (same as we have set up for origination-server), which would have allowed you to test the heroku setup without manually deploying to the staging environment. Ideally we would only deploy to staging what has been reviewed, approved, and merged but not a major concern atm.

COPY . .

# Install indexer
RUN pip3 install poetry==1.6.1
Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth adding a note to the Install indexer comment about the version and use of export.

# Install indexer
RUN pip3 install poetry==1.6.1
RUN poetry install
RUN poetry export --without-hashes --format=requirements.txt > requirements.txt
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above, maybe worth making note of this within the file.

heroku.yml Outdated Show resolved Hide resolved
@wgwz
Copy link
Contributor Author

wgwz commented Oct 19, 2023

We could set up review apps in heroku (same as we have set up for origination-server), which would have allowed you to test the heroku setup without manually deploying to the staging environment. Ideally we would only deploy to staging what has been reviewed, approved, and merged but not a major concern atm.

👍

@wgwz wgwz merged commit 19014bc into main Oct 19, 2023
1 check passed
@wgwz wgwz deleted the kyle/24-heroku-migrations branch October 19, 2023 02:09
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.

Adjust heroku build to run migrations script
3 participants