Page MenuHomePhabricator

Add SFTP solid
ClosedPublic

Authored by natekupp on Aug 4 2019, 12:45 AM.

Details

Reviewers
max
alangenfeld
Group Reviewers
Restricted Project
Commits
R1:80aa58926c38: Add SFTP solid
Summary

Depends on D760.

Implements a working SFTP solid (ported from Airflow)

Test Plan

unit

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

natekupp created this revision.Aug 4 2019, 12:45 AM
natekupp retitled this revision from Dagster add SFTP solid to [WIP] Dagster add SFTP solid.Aug 4 2019, 12:46 AM
natekupp edited the summary of this revision. (Show Details)
natekupp added a reviewer: sashank.
natekupp retitled this revision from [WIP] Dagster add SFTP solid to Add SFTP solid.Aug 4 2019, 3:23 PM
natekupp edited the summary of this revision. (Show Details)
natekupp edited reviewers, added: Restricted Project; removed: sashank.
natekupp edited the summary of this revision. (Show Details)
natekupp added a parent revision: D760: Add Dagster SSH resource.
alangenfeld requested changes to this revision.Aug 5 2019, 4:27 PM
alangenfeld added a subscriber: alangenfeld.
alangenfeld added inline comments.
python_modules/libraries/dagster-ssh/dagster_ssh/solids.py
48

ssh_hook -> ssh_resource

52–62

this feels unnecessarily cute

100–103

use six.raise_from if we are going to catch and add context to an exception

120–127

not obvious to me what is going on here - comments please

This revision now requires changes to proceed.Aug 5 2019, 4:27 PM
max added a subscriber: max.Aug 5 2019, 5:15 PM
max added inline comments.
python_modules/libraries/dagster-ssh/dagster_ssh/resources.py
97

I think we would be well served by aligning our API with the underlying paramiko API

python_modules/libraries/dagster-ssh/dagster_ssh/solids.py
70

update this log message for non-Airflow

80

prefer dagster.utils.mkdir_p

87

I think we should remove these log prefixes

115

what if the remote is windows?

122

I think this is an error in the Airflow codebase -- on python 2.7 I believe paramiko will raise IOError here

natekupp marked 8 inline comments as done.Aug 5 2019, 10:31 PM
natekupp added inline comments.
python_modules/libraries/dagster-ssh/dagster_ssh/resources.py
97

totally agree, I think the Airflow version was trying to be too cute on top of the underlying APIs. if you don't mind, I'll take that on in a follow-up? I'd like to do that for both this client and the tunnel client which'll be a bigger change

python_modules/libraries/dagster-ssh/dagster_ssh/solids.py
52–62

ha fair, I've uncuted it

115

hmmm is SFTP on windows a thing? also does SFTP just do the right thing for '/' even on a windows server?

120–127

exceptions as control flow! added a comment

122

Yeah looks like you're right, but actually seems like all versions raise IOError: http://docs.paramiko.org/en/2.6/api/sftp.html#paramiko.sftp_client.SFTPClient.chdir

natekupp updated this revision to Diff 3416.Aug 5 2019, 10:31 PM
natekupp marked 3 inline comments as done.

up

alangenfeld resigned from this revision.Aug 5 2019, 11:22 PM

ill let max do final sign off

max accepted this revision.Aug 6 2019, 7:23 PM
max added inline comments.
python_modules/libraries/dagster-ssh/dagster_ssh/resources.py
97

ok, can we track in an issue?

python_modules/libraries/dagster-ssh/dagster_ssh/solids.py
115

looking around, it seems to be sftp-server specific. honestly, all of this _make_intermediate_dirs machinery seems like a step too far -- it's not natural to SFTP

122

on python 3, IOError is an OSError

This revision is now accepted and ready to land.Aug 6 2019, 7:23 PM
natekupp updated this revision to Diff 3457.Aug 6 2019, 8:23 PM

remove create_intermediate_dirs per discussion w/ Max

This revision was automatically updated to reflect the committed changes.