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

Add docker dev environment #28

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Add docker dev environment #28

merged 1 commit into from
Nov 1, 2024

Conversation

relud
Copy link
Member

@relud relud commented Nov 1, 2024

adds devcontainer and various make commands to match antenna and socorro

@relud relud requested a review from a team as a code owner November 1, 2024 17:02

.PHONY: test
test: .env .docker-build ## | Run tests.
docker compose run --rm shell bin/test.sh
Copy link
Contributor

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://just.systems/

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.

Copy link
Member Author

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 \
Copy link
Contributor

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.

Copy link
Contributor

@willkg willkg left a 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.

@relud relud added this pull request to the merge queue Nov 1, 2024
Merged via the queue into main with commit 4a78aad Nov 1, 2024
3 checks passed
@relud relud deleted the relud-add-dev-env branch November 1, 2024 18:02
Copy link

@biancadanforth biancadanforth left a 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.

Comment on lines +46 to +47
# Run outside docker because we are testing the matrix python version
PATH="venv/bin:$PATH" bin/test.sh

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:

python-version: ["3.11", "3.12"]
.

Comment on lines +11 to +12
# This should be called from inside a container and after the dependent
# services have been launched. It depends on:

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:

env:
SENTRY_DSN: http://public@localhost:8090/1
STORAGE_EMULATOR_HOST: http://localhost:8001
PUBSUB_EMULATOR_HOST: localhost:5010

Copy link
Member Author

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.

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.

3 participants