build: pip wheel must fail when it does not build a python module #3065
Conversation
ghost
added
It takes longer but works all the time. Uploading an image to a docker repository to speed-up the build requires an additional manual step and access to the docker repository. A possible optimization (would be a few minutes faster) could be implemented by storing docker layers, similar to what is done for the Dockerfile used when testing.
ghost
force-pushed
the
wip-dachary-wheelhouse-fail
branch
from
54820d8 to
e598c92
Compare
|
@msheiny this is modifying something you did, could you please disagree with me ? :-D |
Codecov Report
@@ Coverage Diff @@
## develop #3065 +/- ##
========================================
Coverage 88.22% 88.22%
========================================
Files 32 32
Lines 1852 1852
Branches 212 212
========================================
Hits 1634 1634
Misses 168 168
Partials 50 50Continue to review full report at Codecov.
|
We should include setting up the |
There was a problem hiding this comment.
I know this is outside my assigned area of responsibility, but I had a slow day at work of building and rebuilding containers, so I was able to very the behavior matches the test criteria @dachary described in the PR.
|
@heartsucker thanks for the quick review :-) |
|
@msheiny you cool with this one? |
|
checking @redshiftzero ... |
| pip wheel \ | ||
| -r {{ securedrop_code_filtered }}/requirements/securedrop-app-code-requirements.txt \ | ||
| -w {{ securedrop_app_code_deb_dir }}/var/securedrop/wheelhouse 2>&1 | tee /tmp/w.out | ||
| ! grep -i --quiet 'Failed to build' /tmp/w.out |
There was a problem hiding this comment.
Some super fancy ansible pros would gasp at the usage of error handling like this.... luckily, I think this is fine to do this here ;) Ive seen others break this logic up over two tasks (one task runs, the other task checks the output of the first and fails conditionally with text present). I honestly think the way you went makes more sense.
molecule/builder/Dockerfile
Outdated
| @@ -1,4 +1,5 @@ | |||
| FROM ubuntu:14.04 | |||
| # ubuntu:14.04 as of 2017-12-15 | |||
| FROM ubuntu@sha256:084989eb923bd86dbf7e706d464cf3587274a826b484f75b69468c19f8ae354c | |||
There was a problem hiding this comment.
I don't like embedding the hash right into the Dockerfile. It's more annoying to over-ride programtically in the future if need be. I got a couple commits coming in with some slightly tweaked logic.
There was a problem hiding this comment.
cool. Note that it's also used in other Docker files so maybe you'd like to fix in a uniform way.
Easier to maintain if we need to add additional platforms.
Ansible has some shoddy detection logic for determining when to rebuild an image or layers. I've seen issues in the past with `force: yes` and highly recommend we peg to file hashes as an intermediate way to trigger when to kick off a new docker image.
|
@msheiny 👍 |
Status
Ready for review
Description of Changes
It's not a catastrophy to not fail when pip wheel fails because it will eventually fail when installed. But it's non-intuitive to see a failure during installation and much easier to debug if it fails early.
Testing
securedrop/requirements/securedrop-app-code-requirements.txtDeployment
N/A