Support for SFTP as file location #1481

rajaths010494 · 2022-12-22T06:42:59Z

related-to: #969

Please describe the feature you'd like to see
It is a very common ETL operation to either push data to or load data from FTP sources

Describe the solution you'd like
SFTP should be valid location types in File

Are there any alternatives to this feature?
Implementors otherwise need to use workarounds like S3toSFTPOperator

Acceptance Criteria

All checks and tests in the CI should pass
Unit tests (90% code coverage or more, once available)
Integration tests (if the feature relates to a new database or external service)
Example DAG
Docstrings in reStructuredText for each of methods, classes, functions and module-level attributes (including Example DAG on how it should be used)
Exception handling in case of errors
Logging (are we exposing useful information to the user? e.g. source and destination)
Improve the documentation (README, Sphinx, and any other relevant)
How to use Guide for the feature (example)

codecov · 2022-12-22T07:12:46Z

Codecov Report

Base: 97.43% // Head: 93.84% // Decreases project coverage by -3.58% ⚠️

Coverage data is based on head (4038b6e) compared to base (784223c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1481      +/-   ##
==========================================
- Coverage   97.43%   93.84%   -3.59%     
==========================================
  Files          21       85      +64     
  Lines         740     4175    +3435     
  Branches        0      416     +416     
==========================================
+ Hits          721     3918    +3197     
- Misses         19      176     +157     
- Partials        0       81      +81

Impacted Files	Coverage Δ
python-sdk/src/astro/constants.py	`93.75% <100.00%> (ø)`
python-sdk/src/astro/exceptions.py	`100.00% <100.00%> (ø)`
python-sdk/src/astro/files/base.py	`100.00% <100.00%> (ø)`
python-sdk/src/astro/files/locations/base.py	`76.76% <100.00%> (ø)`
python-sdk/src/astro/files/locations/sftp.py	`100.00% <100.00%> (ø)`
python-sdk/src/astro/sql/operators/load_file.py	`96.90% <0.00%> (ø)`
python-sdk/src/astro/sql/operators/transform.py	`87.09% <0.00%> (ø)`
python-sdk/src/astro/custom_backend/serializer.py	`54.32% <0.00%> (ø)`
python-sdk/src/astro/sql/__init__.py	`95.83% <0.00%> (ø)`
... and 58 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

python-sdk/example_dags/example_load_file.py

python-sdk/noxfile.py

python-sdk/src/astro/files/locations/sftp/sftp.py

python-sdk/example_dags/example_load_file.py

utkarsharma2 · 2022-12-23T07:27:16Z

I think we should also support export_file() operator.

pankajastro

@rajaths010494 do you have plan to add integration tests also?

python-sdk/noxfile.py

python-sdk/src/astro/files/locations/sftp/sftp.py

.github/workflows/ci-python-sdk.yaml

sunank200

There is a lot of common code on this PR and FTP. Do you think we would have a base class for that instead?

.github/workflows/ci-python-sdk.yaml

python-sdk/src/astro/files/locations/sftp/sftp.py

rajaths010494 · 2022-12-23T10:43:58Z

I think we should also support export_file() operator.

Currently smart_open doesn't support write. SFTP hook supports store_file from local_file to remote_file. For this we need a local_file to be created.
Let me explore some other ways as well.

python-sdk/noxfile.py

python-sdk/pyproject.toml

python-sdk/tests/files/locations/test_sftp.py

pankajastro

LGTM

python-sdk/example_dags/example_load_file.py

sunank200

Added comments

python-sdk/src/astro/files/base.py

python-sdk/src/astro/files/locations/sftp.py

python-sdk/tests/files/locations/test_sftp.py

python-sdk/tests_integration/files/locations/test_sftp.py

python-sdk/tests_integration/sql/operators/test_load_file.py

rajaths010494 · 2023-01-03T10:52:45Z

Also, the deepsource is failing. It needs a fix. @rajaths010494

fixed it

Add alternative approach to sftp

rajaths010494 · 2023-01-03T11:37:43Z

Can you test this PR also on https://pypi.org/project/apache-airflow-providers-sftp/4.2.1rc2/ and make sure all the test are passing?

Tested new RC release on my local. It is working fine.

sunank200

LGTM

pankajastro · 2023-01-03T12:22:06Z

Awesome, thanks @rajaths010494 for addressing all the feedback.

related-to: #969 **Please describe the feature you'd like to see** It is a very common ETL operation to either push data to or load data from FTP sources **Describe the solution you'd like** SFTP should be valid location types in `File` **Are there any alternatives to this feature?** Implementors otherwise need to use workarounds like `S3toSFTPOperator` **Acceptance Criteria** - [x] All checks and tests in the CI should pass - [x] Unit tests (90% code coverage or more, [once available](#191)) - [x] Integration tests (if the feature relates to a new database or external service) - [x] Example DAG - [x] Docstrings in [reStructuredText](https://peps.python.org/pep-0287/) for each of methods, classes, functions and module-level attributes (including Example DAG on how it should be used) - [x] Exception handling in case of errors - [x] Logging (are we exposing useful information to the user? e.g. source and destination) - [x] Improve the documentation (README, Sphinx, and any other relevant) - [x] How to use Guide for the feature ([example](https://airflow.apache.org/docs/apache-airflow-providers-postgres/stable/operators/postgres_operator_howto_guide.html)) Co-authored-by: Utkarsh Sharma <utkarsharma2@gmail.com>

rajaths010494 changed the title ~~SFTP support for location~~ Support for FTP as file location Dec 22, 2022

rajaths010494 marked this pull request as ready for review December 22, 2022 07:38

rajaths010494 requested review from dimberman, tatiana, utkarsharma2, sunank200, pankajastro, pankajkoti, feluelle and kaxil as code owners December 22, 2022 07:38

rajaths010494 mentioned this pull request Dec 22, 2022

Support SFTP as file location #1360

Closed

9 tasks

rajaths010494 changed the title ~~Support for FTP as file location~~ Support for SFTP as file location Dec 22, 2022

phanikumv reviewed Dec 22, 2022

View reviewed changes

python-sdk/example_dags/example_load_file.py Outdated Show resolved Hide resolved

python-sdk/noxfile.py Show resolved Hide resolved

python-sdk/src/astro/files/locations/sftp/sftp.py Outdated Show resolved Hide resolved

utkarsharma2 reviewed Dec 22, 2022

View reviewed changes

python-sdk/src/astro/files/locations/sftp/sftp.py Outdated Show resolved Hide resolved

utkarsharma2 reviewed Dec 22, 2022

View reviewed changes

python-sdk/example_dags/example_load_file.py Show resolved Hide resolved

pankajastro reviewed Dec 23, 2022

View reviewed changes

python-sdk/noxfile.py Show resolved Hide resolved

python-sdk/src/astro/files/locations/sftp/sftp.py Outdated Show resolved Hide resolved

.github/workflows/ci-python-sdk.yaml Outdated Show resolved Hide resolved

sunank200 reviewed Dec 23, 2022

View reviewed changes

.github/workflows/ci-python-sdk.yaml Outdated Show resolved Hide resolved

python-sdk/src/astro/files/locations/sftp/sftp.py Outdated Show resolved Hide resolved

feluelle reviewed Dec 25, 2022

View reviewed changes

python-sdk/noxfile.py Show resolved Hide resolved

python-sdk/pyproject.toml Show resolved Hide resolved

python-sdk/tests/files/locations/test_sftp.py Outdated Show resolved Hide resolved

rajaths010494 force-pushed the sftp_location branch 2 times, most recently from 75280e3 to b919e00 Compare December 29, 2022 07:26

pankajastro reviewed Dec 29, 2022

View reviewed changes

python-sdk/example_dags/example_load_file.py Show resolved Hide resolved

pankajastro requested review from feluelle, utkarsharma2 and sunank200 December 29, 2022 08:43

rajaths010494 force-pushed the sftp_location branch from 2dc308a to 1645dcf Compare December 29, 2022 09:18

sunank200 reviewed Dec 29, 2022

View reviewed changes

pankajastro approved these changes Dec 30, 2022

View reviewed changes

rajaths010494 and others added 22 commits January 3, 2023 17:03

SFTP support for location

92b9eb1

Fix test case

d75d22e

Fix review comments

c25b458

Move path of sftp

375146f

Add integration test

fd7c934

SFTP export file

44f0830

Remove skipcq

2a097ca

Fix deep source

5ca2b0b

Add export file integration test

7c54dfa

Update secrets and conn_id changes

4657131

git secrets change

4aa5db4

Revert secrets

b937747

Fix test case

3aa6fc4

Link example SFTP for load file docstring

d70d9aa

modify tests to run for all db

755bcf4

Add test for all filetypes

4c2aea1

Add alternative approch to sftp (#1502)

8282cea

Add alternative approach to sftp

Fix failing test cases

a5e66f4

Add docstrings and fix deep source

5e02d46

Use context manager to load buffer

8ba240b

Rename read_buffer to sftp_stream

59de43f

add test for stream clsoe

4038b6e

rajaths010494 force-pushed the sftp_location branch from f5416b6 to 4038b6e Compare January 3, 2023 11:33

sunank200 approved these changes Jan 3, 2023

View reviewed changes

pankajastro merged commit aaecb75 into main Jan 3, 2023

pankajastro deleted the sftp_location branch January 3, 2023 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for SFTP as file location #1481

Support for SFTP as file location #1481

rajaths010494 commented Dec 22, 2022 •

edited

codecov bot commented Dec 22, 2022 •

edited

utkarsharma2 commented Dec 23, 2022

pankajastro left a comment

sunank200 left a comment

rajaths010494 commented Dec 23, 2022

pankajastro left a comment

sunank200 left a comment

rajaths010494 commented Jan 3, 2023

rajaths010494 commented Jan 3, 2023

sunank200 left a comment

pankajastro commented Jan 3, 2023

Support for SFTP as file location #1481

Support for SFTP as file location #1481

Conversation

rajaths010494 commented Dec 22, 2022 • edited

codecov bot commented Dec 22, 2022 • edited

Codecov Report

utkarsharma2 commented Dec 23, 2022

pankajastro left a comment

Choose a reason for hiding this comment

sunank200 left a comment

Choose a reason for hiding this comment

rajaths010494 commented Dec 23, 2022

pankajastro left a comment

Choose a reason for hiding this comment

sunank200 left a comment

Choose a reason for hiding this comment

rajaths010494 commented Jan 3, 2023

rajaths010494 commented Jan 3, 2023

sunank200 left a comment

Choose a reason for hiding this comment

pankajastro commented Jan 3, 2023

rajaths010494 commented Dec 22, 2022 •

edited

codecov bot commented Dec 22, 2022 •

edited