Skip to content
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.

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

Added --delete flag to timer create transfer #1017

Merged
merged 2 commits into from
Aug 2, 2024

Conversation

derek-globus
Copy link
Contributor

@derek-globus derek-globus commented Aug 1, 2024

What?

Added a --delete flag to globus timer create transfer to match the one in globus transfer (docs).

Note

This flag does not perform a "deletion" task but rather performs a "transfer" task with "delete_destination_extra" set to true.

From the docs, this field requests that the task:

Delete files, directories, and symlinks in the destination directory which don’t exist on the source directory or are a different type. Only applies for recursive directory transfers. Default: false.

I chose to use the flag name --delete instead of --delete-destination-extra to maintain consistency with the regular transfer cli command.

Testing

Created a timer which ran once, performing a transfer that delete extra files on the destination. Observed timer run and transfer task success.

globus timer create transfer --delete $MY_GCP:/~/gcp/subdir/ $MY_GCP:/~/gcp/subdir2/ --stop-after-runs=1

Copy link

@m1yag1 m1yag1 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I added a comment about the command name and the help but I defer to your expertise in what things should be named in the CLI and what would be considered a good help message.

@@ -108,6 +98,15 @@ def resolve_optional_local_time(
help="Stop running the transfer after this number of runs have happened.",
)
@mutex_option_group("--stop-after-date", "--stop-after-runs")
@click.option(
"--delete",
Copy link

@m1yag1 m1yag1 Aug 1, 2024

Choose a reason for hiding this comment

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

I wonder if this should be named "--delete-destination-extra" instead of simply "--delete" to keep the explict-ness.

Additionally, in the help section, it's unclear what "extraneous files in the destination directory" means. Perhaps, using what's in the transfer docs would be better?

Used to provide "mirroring" by deleting any files in the destination directory not in the source.

Copy link

@m1yag1 m1yag1 Aug 1, 2024

Choose a reason for hiding this comment

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

Oh just read your comment again and saw you chose specifically to use --delete for consistency reasons.👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I definitely think that delete-destination-extra is a much better flag than delete. I just don't want this command to diverge at all from the other one.

How about we add this & I'll file a story to deprecate --delete in favor of --delete-destination-extra. We'll need to support a long deprecation tail on that keyword no matter what but this lets us sync up the commands as they are right now.


For the docs, I'm happy to update. I'll update the docs for globus transfer --delete as well though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the shorttext in 3524f82.

It's not identical to the transfer docs you linked cause it's missing some of the surrounding context in that style of doc. Lemme know what you think.

@derek-globus derek-globus merged commit 0237dd0 into globus:main Aug 2, 2024
17 checks passed
@derek-globus derek-globus deleted the timer-transfer-delete branch August 2, 2024 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants