Fix frontend sentry configuration #1362
Conversation
| ENVIRONMENT: str({ | ||
| choices: ["local", "docker", "production"], | ||
| default: "production", | ||
| }), |
There was a problem hiding this comment.
I think this variable was confusing. The "docker" option used to be necessary for something, but at this point in our timeline, it isn't used for much.
The only thing this variable was used for is "Should webpack load env files itself"—that's usually not necessary because docker handles it. It is useful if you're running webpack locally outside of docker, so LOAD_ENV_FILES handles that now.
There was a problem hiding this comment.
Yes - better to provide flags for specific behaviors where there is little distinction for environments / sets of values.
Is API_DEV_PROXY_BASE_URL still a thing?
There was a problem hiding this comment.
It is, though as you've mentioned there are issues running against RC with an authenticated user.
frontends/mit-open/webpack.config.js
Outdated
| ENVIRONMENT !== "local" && WEBPACK_ANALYZE === "True" | ||
| NODE_ENV === "local" && WEBPACK_ANALYZE === "True" |
There was a problem hiding this comment.
This should have been based on NODE_ENV all along, since that's the canonical way to control whether to run a development build or a production-quality (minification, etc) build.
There was a problem hiding this comment.
In Node.js the convention is that "production" instructs that we are not in development mode. Any other value indicates dev mode. It's more common to see "development", as "local" is an environment - as you say, NODE_ENV is really specifying a mode, not an environment.
Better then to say NODE_ENV !== "production" here. Where do we want / not want this to run though - is the change intended to invert?
There was a problem hiding this comment.
Whoops, yes, that was a mistake, thanks. I'm actually inclined to make this check only on WEBPACK_ANALYZE. It's useful to analyze in both production and dev builds.
There was a problem hiding this comment.
More importantly - WEBPACK_ANALYZE will be a boolean from envalid, so that will never be set (in main).
There was a problem hiding this comment.
I'm actually inclined to make this check only on WEBPACK_ANALYZE. It's useful to analyze in both production and dev builds.
Agree, the flag is sufficient. I'd only add extra checks if it specifically can't run there for risk of breakage etc.
There was a problem hiding this comment.
I don't even really think there's any disadvantage to doing it in production other than generating a useless (on server) report.html file.
| # Get VERSION from settings.py | ||
| grep -Eo -m 1 'VERSION\s+= .*(\d+)\.\d+\.\d+.*' main/settings.py | | ||
| head -1 | | ||
| grep -Eo "\d+\.\d+\.\d+" |
There was a problem hiding this comment.
This is not ideal. With VERSION defined in settings.py, it's hard to access outside of Python. (e.g., in our JS
build github action).
For using VERSION during the github action, the options I see are:
- Add a new job to the currently JS-only action. It would be a python job, install our deps, run
./manage.py shelland printsettings.VERSION, then export that for re-use in the next job that actually publishes frontend. This seems like a lot of set up to just get the version. - this bash script
- Update Doof so that it also writes the version to something like
version.txt
(3) would be my preference, though I know we are reluctant to update doof much.
There was a problem hiding this comment.
If settings.py is canonical, it's ok to extract it from there (by any means!). I agree that 3 is nicer. It would be useful to also write this to a version.txt file for the front end build as we do with:
- name: Write commit SHA to file
run: echo $GITHUB_SHA > frontends/mit-open/build/static/hash.txt
ChristopherChudzicki
added
the
Needs Review
| POSTHOG_PROJECT_ID: ${{ secrets.POSTHOG_PROJECT_ID_PROD }} | ||
| POSTHOG_PROJECT_API_KEY: ${{ secrets.POSTHOG_PROJECT_API_KEY_PROD }} | ||
| SENTRY_DSN: ${{ secrets.SENTRY_DSN_PROD }} | ||
| SENTRY_ENV: ${{ secrets.MITOPEN_ENV_PROD }} |
There was a problem hiding this comment.
Actions environments would be an improvement here so we are not having env vars that include the environment name - the job would run in the environment context. Outside of the scope of this PR though, given there are others.
There was a problem hiding this comment.
For us, it is quite canonical. All of our repos have it, and Doof updates it automatically during release via https://github.com/mitodl/release-script/blob/b44bb831e78a5450e3b09e502fd8733b454157f0/version.py#L120
frontends/mit-open/webpack.config.js
Outdated
| ) | ||
| .concat( | ||
| ENVIRONMENT !== "local" && WEBPACK_ANALYZE === "True" | ||
| WEBPACK_ANALYZE === "True" |
There was a problem hiding this comment.
This will be a boolean once handled by envalid, never string "True". I think envalid will throw if set as anything other that "true" or "false".
3 participants
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/5109
Description (What does it do?)
This PR fixes sentry configuration during the frontend build, with the help of some new github secrets that Mike added.
How can this be tested?
I think we will need to do a release to actually test this. Some things worth doing:
./scripts/get_version.sh; it should return the app version