Page MenuHomePhabricator

Don't rely on Unix-only features of tempfile.NamedTemporaryFile
ClosedPublic

Authored by max on Jul 19 2019, 4:21 PM.

Details

Summary

Should fix https://github.com/dagster-io/dagster/issues/1582

NamedTemporaryFiles can only be reopened when already open on Unix, not on Windows NT or later.

Test Plan

Unit, manual confirmation of bugfix

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

max created this revision.Jul 19 2019, 4:21 PM
schrockn accepted this revision.Jul 19 2019, 4:22 PM
schrockn added a subscriber: schrockn.

seems like we need custom lint rules for stuff like this.

how do you test this btw?

This revision is now accepted and ready to land.Jul 19 2019, 4:22 PM
schrockn requested changes to this revision.Jul 19 2019, 4:23 PM

actually maybe make a contextmanager variant of safe_tempfile_path?

This revision now requires changes to proceed.Jul 19 2019, 4:23 PM
max added a comment.Jul 19 2019, 4:27 PM

Yeah, happy to make a context manager.

Running our non-core test suites on Azure would have caught this.

max updated this revision to Diff 3052.Jul 19 2019, 4:31 PM

Use a contextmanager

schrockn accepted this revision.Jul 19 2019, 4:32 PM

interesting that was dead code. kk

This revision is now accepted and ready to land.Jul 19 2019, 4:32 PM