Add support for src/common directory #1104

rzats · 2023-03-06T20:17:03Z

Closes #1037.

Prerequisites:

Unless it is a documentation hotfix it should be merged against the dev branch
Branch is up-to-date with the branch to be merged with, i.e. dev
Build is successful
Code is cleaned up and formatted

Summary

Adds support for the src/common directory visible by both acquisition & server and restructures the web Docker container's imports to make them make a little more sense. This includes modifications to the:

ci.yaml workflow
deploy.json
Makefile
Dockerfile

Additionally, this merges the two (identical) logger files from acquisition and server into one, now located in the common directory.

rzats · 2023-03-06T20:28:34Z

@melange396 @krivard this is potentially an extremely breaking change, so if there's any way to test this besides making sure that the usual tests pass (which they do), please let me know!

melange396 · 2023-03-07T14:04:48Z

we can try deploying to staging, which should be a much closer approximation to the production environment than the dev docker images are, right @korlaxxalrok ?

devops/Dockerfile

krivard · 2023-03-07T16:29:42Z

.github/workflows/ci.yaml

@@ -61,7 +61,7 @@ jobs:
        run: |
          docker network create --driver bridge delphi-net
          docker run --rm -d -p 13306:3306 --network delphi-net --name delphi_database_epidata --cap-add=sys_nice delphi_database_epidata
-          docker run --rm -d -p 10080:80 --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" --env "FLASK_PREFIX=/epidata" --network delphi-net --name delphi_web_epidata delphi_web_epidata
+          docker run --rm -d -p 10080:80 --env "MODULE_NAME=delphi.epidata.server.main" --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" --env "FLASK_PREFIX=/epidata" --network delphi-net --name delphi_web_epidata delphi_web_epidata


question: where is MODULE_NAME used?

its used in the base image that gets built upon in devops/Dockerfile, "tiangolo/meinheld-gunicorn". see the documentation for that image, which i also mentioned in the associated issue (#1037) for this pr.

comments noting the purpose of MODULE_NAME along with that documentation link above should probably be included here, in the Makefile, and in devops/Dockerfile.

MODULE_NAME will also need to be populated in automation or ansible or whatever it is that runs the API server code, which we will probably see when we try to test running this on our staging and/or qa setups. (cc: @korlaxxalrok )

Yep, looks like we'd need to pass an additional environment variable.

melange396

can we move CovidcastRow into common as well? that could help with #1103, as referenced in #1078.

deploy.json

devops/Dockerfile

melange396 · 2023-03-10T22:38:10Z

.github/workflows/ci.yaml

@@ -61,7 +61,7 @@ jobs:
        run: |
          docker network create --driver bridge delphi-net
          docker run --rm -d -p 13306:3306 --network delphi-net --name delphi_database_epidata --cap-add=sys_nice delphi_database_epidata
-          docker run --rm -d -p 10080:80 --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" --env "FLASK_PREFIX=/epidata" --network delphi-net --name delphi_web_epidata delphi_web_epidata
+          docker run --rm -d -p 10080:80 --env "MODULE_NAME=delphi.epidata.server.main" --env "SQLALCHEMY_DATABASE_URI=mysql+mysqldb://user:pass@delphi_database_epidata:3306/epidata" --env "FLASK_SECRET=abc" --env "FLASK_PREFIX=/epidata" --network delphi-net --name delphi_web_epidata delphi_web_epidata


its used in the base image that gets built upon in devops/Dockerfile, "tiangolo/meinheld-gunicorn". see the documentation for that image, which i also mentioned in the associated issue (#1037) for this pr.

comments noting the purpose of MODULE_NAME along with that documentation link above should probably be included here, in the Makefile, and in devops/Dockerfile.

MODULE_NAME will also need to be populated in automation or ansible or whatever it is that runs the API server code, which we will probably see when we try to test running this on our staging and/or qa setups. (cc: @korlaxxalrok )

korlaxxalrok · 2023-03-13T17:14:41Z

This looks good to me. Since this touches both acquisition and the API container image we should coordinate and test fully on staging.

korlaxxalrok

Looks good to me (pending suggestions + any remaining questions) 👍

rzats · 2023-03-15T19:23:56Z

@korlaxxalrok @melange396 @krivard applied the changes, re-requesting review!

(Also, @korlaxxalrok, let me know how and when you'd like to test this on staging)

tests/acquisition/covidcast/test_covidcast_row.py

.github/workflows/ci.yaml

rzats · 2023-03-15T21:47:25Z

Both changes applied! @melange396

melange396

wonderful!

korlaxxalrok

Yep, looks. We can merge and test on staging next.

rzats requested review from krivard and melange396 and removed request for krivard March 6, 2023 20:17

krivard requested a review from korlaxxalrok March 6, 2023 21:56

krivard reviewed Mar 7, 2023

View reviewed changes

melange396 requested changes Mar 10, 2023

View reviewed changes

korlaxxalrok previously approved these changes Mar 13, 2023

View reviewed changes

rzats added 4 commits March 15, 2023 19:11

Add support for src/common directory

b96b028

delete extra logger

2a5d2af

add to dates too

a53bdae

update review comments & covidcastrow

2b2da06

rzats dismissed korlaxxalrok’s stale review via 2b2da06 March 15, 2023 17:16

rzats force-pushed the rzatserkovnyi/common-package branch from 5c4c569 to 2b2da06 Compare March 15, 2023 17:16

two more imports

4df5aec

rzats requested review from krivard, melange396 and korlaxxalrok March 15, 2023 19:24

melange396 requested changes Mar 15, 2023

View reviewed changes

tests/acquisition/covidcast/test_covidcast_row.py Show resolved Hide resolved

.github/workflows/ci.yaml Outdated Show resolved Hide resolved

Review fixes

097e653

rzats requested a review from melange396 March 15, 2023 21:47

melange396 approved these changes Mar 16, 2023

View reviewed changes

korlaxxalrok approved these changes Mar 20, 2023

View reviewed changes

korlaxxalrok merged commit 61e545f into dev Mar 20, 2023

korlaxxalrok deleted the rzatserkovnyi/common-package branch March 20, 2023 16:08

krivard mentioned this pull request Apr 4, 2023

Release Delphi Epidata 0.4.8 #1119

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for src/common directory #1104

Add support for src/common directory #1104

rzats commented Mar 6, 2023 •

edited

Loading

rzats commented Mar 6, 2023

melange396 commented Mar 7, 2023

krivard Mar 7, 2023

melange396 Mar 10, 2023

korlaxxalrok Mar 13, 2023

melange396 left a comment

melange396 Mar 10, 2023

korlaxxalrok commented Mar 13, 2023

korlaxxalrok left a comment

rzats commented Mar 15, 2023

rzats commented Mar 15, 2023

melange396 left a comment

korlaxxalrok left a comment

Add support for src/common directory #1104

Add support for src/common directory #1104

Conversation

rzats commented Mar 6, 2023 • edited Loading

Summary

rzats commented Mar 6, 2023

melange396 commented Mar 7, 2023

krivard Mar 7, 2023

Choose a reason for hiding this comment

melange396 Mar 10, 2023

Choose a reason for hiding this comment

korlaxxalrok Mar 13, 2023

Choose a reason for hiding this comment

melange396 left a comment

Choose a reason for hiding this comment

melange396 Mar 10, 2023

Choose a reason for hiding this comment

korlaxxalrok commented Mar 13, 2023

korlaxxalrok left a comment

Choose a reason for hiding this comment

rzats commented Mar 15, 2023

rzats commented Mar 15, 2023

melange396 left a comment

Choose a reason for hiding this comment

korlaxxalrok left a comment

Choose a reason for hiding this comment

rzats commented Mar 6, 2023 •

edited

Loading