-
Notifications
You must be signed in to change notification settings - Fork 9
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
Updated devcontainer to use production container #4039
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4039 +/- ##
=======================================
Coverage 79.99% 79.99%
=======================================
Files 303 303
Lines 11554 11554
Branches 549 549
=======================================
Hits 9243 9243
Misses 2126 2126
Partials 185 185 ☔ View full report in Codecov by Sentry. |
compscidr
changed the title
Updated devcontainer to be more like production
Updated devcontainer to use production container
Oct 22, 2024
conbrad
approved these changes
Oct 22, 2024
… into jason/devcontainer-updates
dgboss
approved these changes
Oct 23, 2024
Quality Gate passedIssues Measures |
brettedw
approved these changes
Oct 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is part II of the previous PR I made: #4032
In this PR, I updated the Dockerfile.vscode to use the base image we previously published to ghcr. I also tried to make the Dockerfile as close to the production Dockerfile as possible.Based on @conbrad's suggestion, we now use the production container directly.
A few other changes I made:
.env
config to uselocalhost
for the database read/write host and it worked./app
so that when vscode terminal launches you are in the correct folder rather than/workspace
. If you try to runstart.sh
from the workspace, you'll have venv issues, but if you run it from the/app
folder, it will work.api
will be reflected live in the container (otherwise you'd have to rebuild the container to see the changes). Note, I avoided making a workspace mount because otherwise you'll only see the files fromapi
in the vscode editor, and probably you still want to see everything. You could play around with the paths to make this not the case, but it would require updating the paths on the production container.--without dev
install.How I tested:
f1
-> open folder in container, selected thewps
folder.docker compose up db
.env.docker
in/api/app
to.env
and set the db host tolocalhost
(for some reason thehost.docker.internal
didn't work for me.PYTHONPATH=. poetry run alembic upgrade head && poetry run uvicorn app.main:app --host 0.0.0.0 --reload --port 8080
in terminal in vscodestart.sh
script from the/app
directory on the container from the terminal in vscodemake test
Everything ran up to a migration that required me to have auth credentials I don't have.
Caveats:
.venv
folder already existing in yourapp
folder, it will break things. Ideally you should clobber that beforehand. I tried to solve it with a named volume, similar to how people do with thenode_modules
folders, but I had no luck, so perhaps this is a future item.