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

Move testing outside Docker, simplify Dockerfile #508

Merged
merged 6 commits into from
Jan 12, 2023

Conversation

grahamalama
Copy link
Contributor

Addresses #462

In this PR, we move testing outside of Docker. Since the Dockerfile is now only responsible for building a production container, we also simplify the Dockerfile accordingly.

@grahamalama grahamalama added dev Enhancements to development environment docker Pull requests that update Docker code labels Jan 10, 2023
@grahamalama
Copy link
Contributor Author

TODO: do a once-over of our documentation and update what's now outdated

Now that we run tests outside of the Docker container, we now only need
two stages in our multistage build:
- one stage to build Python dependencies
- another stage to copy in the dependencies and run the application
Also, alpha sort make rules
@grahamalama grahamalama force-pushed the tests-outside-container branch from 87f8fd8 to 2dce047 Compare January 11, 2023 16:12
@grahamalama grahamalama force-pushed the tests-outside-container branch from 2dce047 to be58863 Compare January 11, 2023 16:14
@grahamalama grahamalama marked this pull request as ready for review January 11, 2023 16:14
@grahamalama grahamalama requested a review from a team as a code owner January 11, 2023 16:14
Copy link
Contributor

@bsieber-mozilla bsieber-mozilla left a comment

Choose a reason for hiding this comment

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

Woah! Docker is way simpler with this change--thanks @grahamalama !

Makefile Show resolved Hide resolved
bin/update_baseline.sh Show resolved Hide resolved
@grahamalama
Copy link
Contributor Author

Thanks for the review @bsieber-mozilla!

@bkochendorfer, care to give this a once-over before we ship this to staging since this is a pretty big change to how we build the container?

@grahamalama grahamalama removed the dev Enhancements to development environment label Jan 11, 2023
target: "development"
tags: ghcr.io/${{ github.repository }}:${{ github.sha }}

python-version: "3.10"
Copy link
Contributor

Choose a reason for hiding this comment

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

will dependabot notice and be able to update this python-version; or is that something for us to keep in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is something we'll just have to remember, unfortunately. But it would definitely be nice if we could keep these things in sync. Maybe there's a way 🤔

Copy link
Member

@bkochendorfer bkochendorfer left a comment

Choose a reason for hiding this comment

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

This looks good to me. Is there a reason you wanted to move the scripts into bin?

@grahamalama
Copy link
Contributor Author

Is there a reason you wanted to move the scripts into bin?

@bkochendorfer for moving scripts, I just wanted to consolidate all scripts into one place. In fact, in a future PR, I might move scripts from ctms/bin to the top level bin, since I think that would let us leave the PYTHONPATH alone.

As for why I chose bin, I think that was just muscle memory from previous projects.

@grahamalama grahamalama merged commit f7bf386 into main Jan 12, 2023
@grahamalama grahamalama deleted the tests-outside-container branch January 12, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docker Pull requests that update Docker code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants