-
Notifications
You must be signed in to change notification settings - Fork 69
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
cruft removal attempt #1308
base: dev
Are you sure you want to change the base?
cruft removal attempt #1308
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
"// run unit and coverage tests", | ||
{"type": "py3test"} |
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.
removing this will prevent github-deploy-repo
from running local tests during its deployment process
our local wizard, @korlaxxalrok , just successfully tested this on the staging server, so it should be good to go! |
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.
Have a few tangential comments, some just me thinking through consequences and making sure we don't miss anything in the untangling and some ideas for refactors. If you agree with any of my comments, let me know and I can make separate issues.
But overall, looks good and the fact that it runs in staging means 👍
docker run --rm --network delphi-net --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/tests | ||
docker run --rm --network delphi-net \ | ||
--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \ | ||
--mount type=bind,source=./repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \ |
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.
Comment (non-blocking, probably be separate issue): we should move to having CI use the Makefile commands. It would help keep local testing identical with CI and would remove code duplication.
docker run --rm --network delphi-net delphi_web_python python -m pytest --import-mode importlib repos/delphi/delphi-epidata/integrations | ||
docker run --rm --network delphi-net \ | ||
--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \ | ||
--mount type=bind,source=./repos/delphi/delphi-epidata/src,target=/usr/src/app/delphi/epidata,readonly \ |
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.
Question (non-blocking, separate issue): any reason why unit tests and integrations are separated? In the Makefile both are run together, using that command here would remove this Docker command duplication.
|
||
COPY repos/delphi/operations/src delphi/operations | ||
COPY repos/delphi/utils/src delphi/utils | ||
COPY repos/delphi/delphi-epidata/src delphi/epidata |
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.
Comment (non-blocking): this COPY command is later overwritten by a runtime mount between the same folders we run tests. Consider getting rid of this.
Thinking out loud (non-blocking): looks like we're dropping the top-level repos
dir within the Dockerfile. Want to make sure we think through the consequences of this. AFAICT, previously we copied the entire local repos
folder over into /usr/src/app/repos
and then moved pieces over to /usr/src/app/delphi/
and /usr/src/app/undefx/
. So it looks like we're skipping the intermediate step. So is there anything that still relies on that repos
folder? The only thing I can find are the Docker testing commands that end with e.g. python -m pytest --import-mode importlib repos/delphi/delphi-epidata/integrations
. But that is handled by the first mount command in those Docker commands:
--mount type=bind,source=./repos/delphi/delphi-epidata,target=/usr/src/app/repos/delphi/delphi-epidata,readonly \
So the edit to my first sentence from this sequence of thoughts is: we're not dropping the repos
dir, it just gets added by a mount at runtime.
Comment (non-blocking, separate issue): it's tough to mentally imagine the directory structure of Dockerfile when runtime mounts are involved (due to non-locality between Docker run command and the Dockerfile). Maybe we should add a comment at the end of this Dockerfile saying something like "at runtime, these additional directories are present".
Comment (non-blocking, separate issue): a future future solution to this non-locality is to make a unified docker-compose.yml
file.
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.
Most of my review suggestions are addressed by #1315
An attempt to remove a bunch of old cruft from our dependencies, tests, and builds...
Summary:
undefx/py3tester
,undefx/undef-analysis
,cmu-delphi/github-deploy-repo
,cmu-delphi/flu-contest
, andcmu-delphi/nowcast
) from the github actions for CI & performance tests, thedelphi_web_python
Dockerfile
definition, and the dev env installation script.github-deploy-repo
config (aka./deploy.json
) so we can axepy3tester
junk. We run tests in many other places, so it isnt really necessary for g-d-r to do it too.Dockerfile
for thedelphi_web_python
image, removing need for its old separate bash script (dev/docker/python/setup.sh
)Results:
Caveats:
delphi_web_python
docker image)Precursor to:
acquisition
to an installable package #939Replaces: