Page MenuHomePhabricator

Update integration image
ClosedPublic

Authored by nate on Dec 29 2019, 2:59 AM.

Details

Summary

This diff makes several fixes:

  • Rebase integration image on slim Debian variants (and install a few missing deps we still need, like ssh)
  • Our integration image Dockerfile seems to have broken due to things upstream changing, fixed by this diff
  • Fixes https://github.com/dagster-io/dagster/issues/1999
  • Our snapshotted requirements.txt dependencies for Python 3 were previously based on a Python 3.5 environment, which was increasingly being left behind. This diff updates to use Python 3.7 for snapshotting a requirements.txt file, and adds PEP 496 environment markers to constrain installed dependencies s.t. the 3.5 and 3.8 image builds still succeed.

Final integration image build is here: https://buildkite.com/dagster/integration-image-builds/builds/46

Test Plan

buildkite

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Harbormaster completed remote builds in B6670: Diff 8224.
nate edited the summary of this revision. (Show Details)Dec 29 2019, 6:46 PM
nate updated this revision to Diff 8234.Dec 29 2019, 7:08 PM
nate edited the summary of this revision. (Show Details)

up

Harbormaster failed remote builds in B6682: Diff 8236!
nate edited the summary of this revision. (Show Details)Dec 29 2019, 8:42 PM
Harbormaster failed remote builds in B6688: Diff 8242!
Harbormaster failed remote builds in B6693: Diff 8247!
nate edited the summary of this revision. (Show Details)Dec 29 2019, 9:38 PM
nate edited the summary of this revision. (Show Details)Dec 29 2019, 9:56 PM
nate added a reviewer: schrockn.
nate edited the summary of this revision. (Show Details)
schrockn accepted this revision.Dec 30 2019, 12:04 AM

Cool. Have some commentary but none of it blocking

.buildkite/images/docker/snapshot-update.sh
38

it might be worth commenting as to what these sed commands are actually doing (e.g. I assume that for example it is tacking on the `python_version >= '3.6' on to lines that include black but I don't know that for certain)

38

In terms of stuff like this. I consider this putting "lipstick on a pig" until we have a more principled dependency management system. What would you think about capturing all these in an issue or set of issues so we can track all the things we want to fix if we set someone off to fix this? e.g. I believe we should be able to codegen per-python-version requirements.txt files without having to resort to string munging. Also, e.g., we shouldn't have to include black in these images at all.

That way we can have something greppable that says "you are not done until all of these are gone"

This revision is now accepted and ready to land.Dec 30 2019, 12:04 AM
nate added inline comments.Dec 30 2019, 2:31 AM
.buildkite/images/docker/snapshot-update.sh
38

Good thought, I've updated to include https://github.com/dagster-io/dagster/issues/1295 in the code, and I'll comment on that issue also to point to this diff. I've also added explanation of the sed commands.

Maybe something I'm missing, but is there a reason you think we shouldn't include black in the integration images? if we don't install tools like black, pytest, etc. in these images, we'd have to install them at test runtime, so there's a test performance cost

nate updated this revision to Diff 8250.Dec 30 2019, 2:31 AM
nate edited the summary of this revision. (Show Details)

comments

This revision was automatically updated to reflect the committed changes.
schrockn added inline comments.Dec 30 2019, 3:21 AM
.buildkite/images/docker/snapshot-update.sh
38

black is not run during pytest runs. we just fallback to a uniform integration image in all cases for simplicity so right now in our test run that only does black downloads the entire image when it could download a tiny one . but i don't think we should end up having a single integration image in the end state, but custom ones as appropriate, which in a fully formed dep system would be managed for us.