Refactor PIINVOICE so env vars are fetched at start of billing pipeline. #179
Conversation
QuanMPhm
requested review from
QuanMPhm and
larsks
and removed request for
QuanMPhm
KelvinLinBU
force-pushed
the
refactor-env
branch
3 times, most recently
from
63d9226 to
6d1bdef
Compare
|
@QuanMPhm can be merged with main now |
| - NewPICreditProcessor | ||
| """ | ||
|
|
||
| chrome_binary_location: str |
There was a problem hiding this comment.
Could you move this arg below the constants. In this codebase, the conventional structure is class constants, then variables, then functions
| chrome_binary_location = os.environ.get( | ||
| "CHROME_BIN_PATH", "/usr/bin/chromium" | ||
| ) | ||
| if not os.path.exists(chrome_binary_location): |
There was a problem hiding this comment.
You do not need to move this check to process_report.py. We only want to move the fetching of the env var. Error checking can stay in the invoice class for now.
There was a problem hiding this comment.
You should add CHROME_BIN_PATH in REQUIRED_ENV_VARS
process_report/process_report.py
Outdated
| def main(): | ||
| """Remove non-billable PIs and projects""" | ||
|
|
||
| chrome_binary_location = os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium") |
There was a problem hiding this comment.
Related to the adding CHROME_BIN_PATH to REQUIRED_ENV_VARS, you should have the env var fetched after the env vars have been validated
…ipeline. This makes use of `required_env_vars()`.
QuanMPhm
dismissed
their stale review
I am now a contributor to this PR as well
| subprocess.run( | ||
| [ | ||
| CHROME_BIN_PATH, | ||
| os.environ.get("CHROME_BIN_PATH", "/usr/bin/chromium"), |
There was a problem hiding this comment.
What was the motivation for this change? I think it's generally better practice to read your environment variables early, rather than doing it inline like this.
3 participants
Closes #168. Adhere to testing best practices as outlined in #159 (comment) and #168 (comment). Modify utils.py to accommodate changes.