Improve GCSToSFTPOperator paths handling #11284

TobKed · 2020-10-05T16:04:41Z

Improve GCSToSFTPOperator paths handling.

Now for every file on GCS is created separate folder on SFTP.

now:

copy_file_from_gcs_to_sftp = GCSToSFTPOperator(
        task_id="file-copy-gsc-to-sftp",
        source_bucket="bucket_name",
        source_object="folder/subfolder/file.txt",
        destination_path="/tmp",
    )
# downloads file to "/tmp/folder/subfolder/file.txt"

EDIT:
I made previous behavior default (for backward compatibility) but possible to change it by ignore_gcs_path_in_destination argument:

copy_file_from_gcs_to_sftp = GCSToSFTPOperator(
        task_id="file-copy-gsc-to-sftp",
        source_bucket="bucket_name",
        source_object="folder/subfolder/file.txt",
        destination_path="/tmp",
        keep_directory_structure=False,
    )
# downloads file to "/tmp/file.txt"

github-actions · 2020-10-05T16:05:06Z

The CI and PROD Docker Images for the build are prepared in a separate "Build Image" workflow,
that you will not see in the list of checks (you will see "Wait for images" jobs instead).

You can checks the status of those images in The workflow run

TobKed · 2020-10-05T16:06:07Z

cc @mik-laj @turbaszek @michalslowikowski00 @olchas

turbaszek · 2020-10-05T16:17:15Z

Should we make this configurable to avoid breaking change? I see the point of downloading a single file but this may be problematic when downloading multiple files with the same name (for example structure like output/2020-09-20/data.csv and downloading data for multiple days). WDYT @TobKed ?

olchas · 2020-10-06T10:32:17Z

Should we make this configurable to avoid breaking change? I see the point of downloading a single file but this may be problematic when downloading multiple files with the same name (for example structure like output/2020-09-20/data.csv and downloading data for multiple days). WDYT @TobKed ?

From what I understood by reading the code, only the part up to the wildcard character is stripped from the source_object path. In other word, when downloading multiple files from your example, you would set source_object to something like this: output/*. Then, when uploading them to destination_path, only the output would be stripped from every collected object's path, so you should get multiple directories for multiple days in your destination.

Did I get this right, @TobKed?

TobKed · 2020-10-06T12:44:41Z

@olchas , yes, you are right. In your example of wildcard folders will be created.

@turbaszek I made this behavior optional for backward compatibility. PTAL

michalslowikowski00 · 2020-10-07T07:09:09Z

airflow/providers/google/cloud/transfers/gcs_to_sftp.py

@@ -52,6 +52,9 @@ class GCSToSFTPOperator(BaseOperator):
    :param destination_path: The sftp remote path. This is the specified directory path for
        uploading to the SFTP server.
    :type destination_path: str
+    :param ignore_gcs_path_in_destination: (Optional) When set to False the path of the file


It confused me a little bit. Can you explain this parameter?

For example: If you want to copy file from bucket: gs://bucket_name/data/2020/file.txt to /tmp directory, by default it will be copied reflecting object path on the bucket, so file path on sftp will be /tmp/data/2020/file.txt. If parameter ignore_gcs_path_in_destination will be set to True the file path on sftp will be /tmp/file.txt.

I am open for proposals for better name for this parameter :)

Got it. I do not have better solution.

Can you add an example to the description?

Sure. Example was added.

turbaszek · 2020-10-07T17:20:38Z

airflow/providers/google/cloud/transfers/gcs_to_sftp.py

+            if self.ignore_gcs_path_in_destination:
+                source_object_basename = os.path.basename(self.source_object)
+                destination_path = os.path.join(self.destination_path, source_object_basename)
+            else:
+                destination_path = os.path.join(self.destination_path, self.source_object)


How about

def _resolve_destination_path(self, prefix=None): if self.ignore_gcs_path_in_destination: source_object_basename = os.path.basename(self.source_object, start=prefix) return os.path.join(self.destination_path, source_object_basename) else: return os.path.join(self.destination_path, self.source_object)

then we can avoid duplication of the code

Thanks @turbaszek , it is a great suggestion.
I modified it slightly: 9b9ef60

mik-laj · 2020-10-09T08:22:52Z

airflow/providers/google/cloud/transfers/gcs_to_sftp.py

@@ -52,6 +52,9 @@ class GCSToSFTPOperator(BaseOperator):
    :param destination_path: The sftp remote path. This is the specified directory path for
        uploading to the SFTP server.
    :type destination_path: str
+    :param ignore_gcs_path_in_destination: (Optional) When set to False the path of the file


Suggested change

:param ignore_gcs_path_in_destination: (Optional) When set to False the path of the file

:param use_relative_paths: (Optional) When set to False the path of the file

or keep_directory_structure

I renamed it to keep_directory_structure. Thanks @mik-laj !

michalslowikowski00 · 2020-10-09T08:37:11Z

tests/providers/google/cloud/transfers/test_gcs_to_sftp.py

 DESTINATION_SFTP = "destination_path"


 # pylint: disable=unused-argument
 class TestGoogleCloudStorageToSFTPOperator(unittest.TestCase):
+    @parameterized.expand(


Perfect 💪

mik-laj · 2020-10-12T10:35:19Z

airflow/providers/google/cloud/transfers/gcs_to_sftp.py

+                task_id="file-move-gsc-to-sftp",
+                source_bucket=BUCKET_SRC,
+                source_object=OBJECT_SRC_2,
+                destination_path=DESTINATION_PATH_1,


Can you use values here instead of undefined constants?

I missed it. Thank you! FIxed.

TobKed · 2020-10-12T16:57:00Z

I rebased on the latest master and fixed the logic mistake. Previously i forget that after changing parameter from ignore_gcs_path_in_destination to keep_directory_structure the logic of resolving paths should be inverted.

mik-laj · 2020-10-21T14:33:45Z

We test it together with the customer. Pllease wait for customer feedback.

kaxil · 2020-11-04T00:30:32Z

Can you please rebase your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

github-actions · 2020-11-20T18:18:58Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2020-11-21T16:10:06Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

turbaszek · 2020-11-27T11:43:38Z

@mik-laj any feedback about your tests?

github-actions · 2020-11-27T11:43:41Z

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

mik-laj · 2020-11-27T11:52:00Z

@mik-laj any feedback about your tests?

As far as I know, they were successful. Tobias has more details.

github-actions · 2021-01-21T16:22:52Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-01-21T16:25:29Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-01-21T16:31:48Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

github-actions · 2021-01-21T16:31:59Z

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

marksworn · 2021-02-18T13:08:06Z

This does not appear to be working for me - the documentation is unclear which should be used to exclude the GCS path structure from the destination path - True or False? I have tested with both and with both it is still adding elements of the GCS filepath that I do not want to include.

This is a useful feature in some cases, but it feels like it should be default that the user controls definition of the source and destination paths without any unexpected behaviour.

With keep_directory_structure=False and destination set to /Activity/pending/:

Executing copy of gs://<bucket>/Activity/2021/02/activity_20210217.csv to /Activity/pending/Activity/2021/02/activity_20210217.csv

With keep_directory_structure=True:

Executing copy of gs://<bucket>/Activity/2021/02/activity_20210217.csv to /Activity/pending/Activity/2021/02/activity_20210217.csv

TobKed · 2021-02-22T08:18:31Z

@marksworn I am sorry to hear that. Would like to create issue with link to this PR? Would you like to create fix for it? I would be happy to assist you with this.

boring-cyborg bot added the provider:google Google (including GCP) related issues label Oct 5, 2020

turbaszek added the area:providers label Oct 5, 2020

michalslowikowski00 reviewed Oct 7, 2020

View reviewed changes

TobKed force-pushed the improve-gcs-to-sftp branch from 590be6c to 6b1e58e Compare October 7, 2020 08:39

turbaszek reviewed Oct 7, 2020

View reviewed changes

TobKed force-pushed the improve-gcs-to-sftp branch 2 times, most recently from ec06638 to e7694f4 Compare October 9, 2020 06:26

mik-laj reviewed Oct 9, 2020

View reviewed changes

michalslowikowski00 reviewed Oct 9, 2020

View reviewed changes

TobKed force-pushed the improve-gcs-to-sftp branch 2 times, most recently from 5276964 to 40f928f Compare October 9, 2020 13:04

turbaszek approved these changes Oct 11, 2020

View reviewed changes

turbaszek requested a review from mik-laj October 11, 2020 14:47

mik-laj reviewed Oct 12, 2020

View reviewed changes

TobKed force-pushed the improve-gcs-to-sftp branch from 40f928f to 19d2004 Compare October 12, 2020 16:53

turbaszek approved these changes Oct 21, 2020

View reviewed changes

turbaszek requested a review from mik-laj October 21, 2020 14:17

TobKed force-pushed the improve-gcs-to-sftp branch 2 times, most recently from e85e838 to 0cfa7d9 Compare November 20, 2020 17:47

TobKed force-pushed the improve-gcs-to-sftp branch from 7da3d9b to 8466393 Compare November 21, 2020 15:18

TobKed force-pushed the improve-gcs-to-sftp branch from 8466393 to a3192d4 Compare November 23, 2020 12:35

TobKed requested a review from turbaszek November 27, 2020 11:33

turbaszek approved these changes Nov 27, 2020

View reviewed changes

github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 27, 2020

TobKed mentioned this pull request Dec 14, 2020

Add retryer to SFTP hook connection #13065

Merged

TobKed force-pushed the improve-gcs-to-sftp branch from a3192d4 to f4cde05 Compare January 21, 2021 11:31

mik-laj approved these changes Jan 24, 2021

View reviewed changes

turbaszek approved these changes Feb 1, 2021

View reviewed changes

TobKed force-pushed the improve-gcs-to-sftp branch 2 times, most recently from 7b59717 to 50931c9 Compare February 3, 2021 19:43

Tobiasz Kędzierski added 4 commits February 5, 2021 13:00

Improve GCSToSFTPOperator paths handling

1b95af0

fixup! Improve GCSToSFTPOperator paths handling

6ec3c24

fixup! fixup! Improve GCSToSFTPOperator paths handling

d039aae

fixup! fixup! fixup! Improve GCSToSFTPOperator paths handling

2036748

TobKed force-pushed the improve-gcs-to-sftp branch from 50931c9 to 2036748 Compare February 5, 2021 12:01

potiuk merged commit 5d7d46b into apache:master Feb 5, 2021

potiuk deleted the improve-gcs-to-sftp branch February 5, 2021 19:15

	:param ignore_gcs_path_in_destination: (Optional) When set to False the path of the file
	:param use_relative_paths: (Optional) When set to False the path of the file

Improve GCSToSFTPOperator paths handling #11284

Improve GCSToSFTPOperator paths handling #11284

Uh oh!

Conversation

TobKed commented Oct 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 5, 2020

Uh oh!

TobKed commented Oct 5, 2020

Uh oh!

turbaszek commented Oct 5, 2020

Uh oh!

olchas commented Oct 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobKed commented Oct 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobKed commented Oct 12, 2020

Uh oh!

mik-laj commented Oct 21, 2020

Uh oh!

kaxil commented Nov 4, 2020

Uh oh!

github-actions bot commented Nov 20, 2020

Uh oh!

github-actions bot commented Nov 21, 2020

Uh oh!

turbaszek commented Nov 27, 2020

Uh oh!

github-actions bot commented Nov 27, 2020

Uh oh!

mik-laj commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 21, 2021

Uh oh!

github-actions bot commented Jan 21, 2021

Uh oh!

github-actions bot commented Jan 21, 2021

Uh oh!

github-actions bot commented Jan 21, 2021

Uh oh!

marksworn commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobKed commented Feb 22, 2021

Uh oh!

Uh oh!

TobKed commented Oct 5, 2020 •

edited

Loading

olchas commented Oct 6, 2020 •

edited

Loading

mik-laj Oct 9, 2020 •

edited

Loading

mik-laj commented Nov 27, 2020 •

edited

Loading

marksworn commented Feb 18, 2021 •

edited

Loading