Skip to content

Handle multiple files from a single source when using curl to transfer #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EddieLF wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Handle multiple files from a single source when using curl to transfer #59

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions .github/workflows/transfer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import subprocess
import sys

from references import SOURCES
from references import SOURCES, curl, curl_with_user_agent


def parser(args: list[str]) -> argparse.Namespace:
Expand Down Expand Up @@ -49,13 +49,24 @@ def main(name: str, references_prefix: str, gcp_project: str) -> None:
"""
source = {s.name: s for s in SOURCES}[name]
if source.transfer_cmd and source.src:
cmd = source.transfer_cmd(
src=source.src,
dst=os.path.join(references_prefix, source.dst),
project=gcp_project,
)
print(cmd)
subprocess.run(cmd, shell=True)
if source.files and source.transfer_cmd in (curl, curl_with_user_agent):

@illusional illusional Aug 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, I'd probably tweak this so the transfer_cmd returns a list of commands (or a single string), and then run multiple if it's a list. So we keep how it's run closer to the implementation.

Are there type annotations that are wrong here, or is files not used here, I'm a bit confused what the behaviour is supposed to be here.

@EddieLF EddieLF Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is files not used here

I think files is not used here. There are two types of Source objects in references.py - sources containing a single reference file, for which only src and dst are defined. Then there are sources containing multiple reference files. For these sources, the attribute files is defined as a dict.

When the transfer_cmd is gsutil rsync, we copy the entire src directory to the dst destination, meaning we get what's in the files dict with the correct directory structure (alongside whatever else the rsync picks up from the source), so there's no need to actually look at the files dict.

Whereas for a curl, we need to actually look at all the files in the dict and execute curl command for each of them.

IMO, I'd probably tweak this so the transfer_cmd returns a list of commands (or a single string), and then run multiple if it's a list. So we keep how it's run closer to the implementation.

Can you elaborate? Are you suggesting something like updating this line to

transfer_cmd = [curl_with_user_agent, curl_with_user_agent, curl_with_user_agent]

And then updating the transfer script code to do something like

if source.files and isinstance(source.transfer_cmd, list):
  for file, command in zip(source.files.values(), transfer_cmd):
    cmd = command(...)
    print(command)
    subprocess.run(command, shell=True)

@illusional illusional Aug 21, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I think this behaviour should be decided by the transfer_cmd implementation. It probably makes more sense to pass the Source, and change the transfer_cmd to take the Source object.

RE the list of commands, I mean something like:

# this is a garbage implementation, it's just trying to illustrate the point
def my_transfer_command(source: Source) -> list[str] | str:
    if many_files or a_directory:
        return ["cp {f} ." for f in many_files]
    return "cp one.file ."

# If there are multiple files and we're using curl to transfer, iterate through them
for file in source.files.values():
cmd = source.transfer_cmd(
src=str(os.path.join(source.src, file)),
dst=os.path.join(references_prefix, source.dst, file),
project=gcp_project,
)
print(cmd)
subprocess.run(cmd, shell=True)
else:
cmd = source.transfer_cmd(
src=source.src,
dst=os.path.join(references_prefix, source.dst),
project=gcp_project,
)
print(cmd)
subprocess.run(cmd, shell=True)


if __name__ == '__main__':
Expand Down