-
Notifications
You must be signed in to change notification settings - Fork 400
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
Replace smoke test with a basic test against __frontend_lbheartbeat__ #13390
Conversation
We only really need care to check if the docker image express server works, so we use an endpoint that doesn't require the API (or even the built statics) with a simpler setup. Because it's lighter it can be included in the default job that is ran even on PRs. `tox` is also removed as it's just not necessary anymore.
77aaf0f
to
1929379
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13390 +/- ##
=======================================
Coverage 98.28% 98.28%
=======================================
Files 267 267
Lines 10629 10629
Branches 3241 3241
=======================================
Hits 10447 10447
Misses 169 169
Partials 13 13 ☔ View full report in Codecov by Sentry. |
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.
I have a few questions, generally looks good. One ask also to add the check image logic to a job that runs on PRs so we can let that logic execute successfully, then you can revert back to running only on pushes.
- run: pip install tox | ||
- run: TOXENV=dennis-lint tox | ||
|
||
- run: |
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.
It's not clear why this is changed?
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.
Just because I noticed tox
was not necessary any more with the smoke tests removed, it was never super useful for the check locales step
.circleci/config.yml
Outdated
- run: | ||
name: Check image works | ||
command: | | ||
docker run --rm -p 4000:4000 -d -e NODE_ENV=production -e NODE_CONFIG_ENV=prod addons-frontend |
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.
I wonder if (perhaps) a simple docker compose file with a healthcheck would achieve this in a more reproducible way without adding much complexity.
services:
addons-frontend:
image: IMAGE_NAME
environment:
- NODE_ENV=production
- NODE_CONFIG_ENV=prod
ports:
- "4000:4000"
healthcheck:
test: [
"CMD-SHELL",
"curl -f http://127.0.0.1:4000/__frontend_lbheartbeat__"
]
interval: 10s
timeout: 5s
retries: 10
Maybe that could work? Now you can just run
docker compose up --wait -d
Could be a follow up
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.
That's an option we could look into as a follow-up yeah. I didn't want to introduce docker compose into the mix since we don't currently use it when running the frontend locally (and we're already using manual docker
commands in the CircleCI config)
- run: | ||
name: Check image works | ||
command: | | ||
docker run --rm -p 4000:4000 -d -e NODE_ENV=production -e NODE_CONFIG_ENV=prod addons-frontend |
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.
I'm having issues running this locally. it could be a platform issue, I'm not sure. In debugging I did find that docker supports adding a healthcheck to docker run.
--health-cmd string Command to run to check health
--health-interval duration Time between running the check (ms|s|m|h) (default 0s)
--health-retries int Consecutive failures needed to report unhealthy
--health-start-interval duration Time between running the check during the start period (ms|s|m|h) (default 0s)
--health-start-period duration Start period for the container to initialize before starting health-retries countdown (ms|s|m|h) (default 0s)
--health-timeout duration Maximum time to allow one check to run (ms|s|m|h) (default 0s)
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.
Confirmed working on @diox machine. we will check it in CI after merging and if anything goes wrong we can revert.
We only really need care to check if the docker image express server works, so we use an endpoint that doesn't require the API (or even the built statics) with a simpler setup.
tox
is also removed as it's just not necessary anymore.Fixes mozilla/addons#2108