-
Notifications
You must be signed in to change notification settings - Fork 0
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 docker dev environment #28
Conversation
fbfff0a
to
d085267
Compare
d085267
to
62cf20e
Compare
|
||
.PHONY: test | ||
test: .env .docker-build ## | Run tests. | ||
docker compose run --rm shell bin/test.sh |
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.
As a side note, I've been switching all my personal projects to using just
:
https://github.com/willkg/everett/blob/main/justfile
It adds another requirement, but it's easier to use and better at doing the things I was using Makefiles for. I don't know whether it's easy to install/use on macOS or whether it's a viable replacement for make for our purposes. But I figured I'd mention it.
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.
looks awesome, i'd love to switch. I think we should decide as a team and make the change more broadly than only this repo, so i'll bring it up in the meeting on tuesday
@@ -640,6 +640,10 @@ urllib3==2.2.2 \ | |||
# requests | |||
# sentry-sdk | |||
# twine | |||
urlwait==1.0 \ |
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.
If we have waitfor, we can build into it everything we're using urlwait for. urlwait is pmac's, but I don't think it's maintained anymore.
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 seems fine. I didn't test it out figuring it's a dev environment and we can fix broken things as we discover them.
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 know this already merged, but I had planned to look at it today, so I added a couple of comments mostly for my own understanding of what's going on here.
Works a lot smoother to test PRs like #26 in the future! Though now I know how to do that in the absence of all the docker set up.
# Run outside docker because we are testing the matrix python version | ||
PATH="venv/bin:$PATH" bin/test.sh |
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.
Ah. At first I was wondering "what is The Matrix TM", but I see we are running the tests against multiple versions of Python:
obs-common/.github/workflows/build.yaml
Line 17 in 4a78aad
python-version: ["3.11", "3.12"] |
# This should be called from inside a container and after the dependent | ||
# services have been launched. It depends on: |
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.
Does this comment apply anymore if we're running this outside of a container per the "Run tests" CI job in .github/workflows/build.yaml
?
I see make test
still runs it from a shell in a Docker container. And the environment variables are using docker aliases for the host names... Now I'm wondering how the above CI is passing still in that case. I guess we set different env vars in that context... Ah I see that we do:
obs-common/.github/workflows/build.yaml
Lines 40 to 43 in 4a78aad
env: | |
SENTRY_DSN: http://public@localhost:8090/1 | |
STORAGE_EMULATOR_HOST: http://localhost:8001 | |
PUBSUB_EMULATOR_HOST: localhost:5010 |
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.
yeah, anyone running this manually should do so inside the docker container. CI runs it outside of the docker container so that it can test multiple python versions.
adds devcontainer and various make commands to match antenna and socorro