Add CLI command for batch creation that integrates data syncing

[jonavellecuerdo]

Purpose and background context

A CLI command is required to support the new batch creation step for DSC. These changes also connect the batch creation step with the syncing process:

• Workflow.create_batch can skip data retrieval (e.g. API requests) when --sync-data is provided in the command args
• The create CLI command can invoke the 'sync' command when --sync-data is provided in the command args

A couple thoughts:

• Syncing defaults to False.
  • For Dev and Stage, syncing is not expected.
    *For Prod, syncing the batch data from the S3 bucket in Stage is expected.
• While a workflow like Wiley could very well check if data exists in the S3 bucket before making the API request, passing the synced boolean to the batch creation methods allows DSC to skip unnecessary reads to S3 altogether.

How can a reviewer manually see the effects of these changes?

Review of added unit tests is sufficient for this PR

Includes new or updated dependencies?

NO

Changes expectations for external applications?

YES - This will require changes to the DSC step function. By enabling the create CLI command to invoke the sync CLI command, I expect this will allow us to simplify the logic of our step function and consolidate logs for both commands in a single CloudWatch logstream. For context, the latest version of the step function we were proposing is outlined in jc-test-dsc-with-sync-dev. Ticket IN-1457 will address updates to the step function.

What are the relevant tickets?

• https://mitlibraries.atlassian.net/browse/IN-1456

[jonavellecuerdo]

For workflows like Wiley, the synced boolean can be used to skip data retrieval steps if the data has been synced (e.g., in Prod, the batch "folder" and its contents will be pulled from Stage).

Also, note that while SimpleCSV _does not use the synced argument, the name is not leading with an underscore. This is because mypy will raise an error about the mismatched parameter names between the @abstractmethod and the implemented, and it's suspected that this is because the parameter is a keyword argument. 🤔 Ruff error [ARG002](https://docs.astral.sh/ruff/rules/unused-method-argument/) will need to be skipped as a result.

TLDR: Either mypy or Ruff would have an issue. Decided to ignore Ruff's error.

[ehanson8]

A few comments but looking good!

[ehanson8]

Perhaps these should be prepended with sync_ for greater clarity?

[ghukill]

Agreed with @ehanson8 here (or sync- if we're going with dashes in CLI args).

[ehanson8]

Yes, sync-!

[jonavellecuerdo]

I updated the signature of the create CLI command as follows:

@main.command()
@click.pass_context
@click.option("--sync-data/--no-sync-data", default=False)
@click.option(
    "--sync-dry-run",
    is_flag=True,
    help=(
        "Display the operations that would be performed using the "
        "sync command without actually running them"
    ),
)
@click.option(
    "-s",
    "--sync-source",
    help=(
        "Source directory formatted as a local filesystem path or "
        "an S3 URI in s3://bucket/prefix form"
    ),
)
@click.option(
    "-d",
    "--sync-destination",
    help=(
        "Destination directory formatted as a local filesystem path or "
        "an S3 URI in s3://bucket/prefix form"
    ),
)
@click.option(
    "-e",
    "--email-recipients",
    help="The recipients of the batch creation results email as a comma-delimited string",
    default=None,
)
def create(
    ctx: click.Context,
    *,
    sync_data: bool = False,
    sync_dry_run: bool = False,
    sync_source: str | None = None,
    sync_destination: str | None = None,
    email_recipients: str | None = None,
) -> None:

Note: By moving the asterisk (*) to after ctx: click.Context, the args can be arranged in a more logical order (but it does mean if create is ever called outside of the pipenv command or not as a CLI, user must provide all params as keyword arguments (doesn't seem like a bad call given the number of params anyway).

[ghukill]

A great start. I left a couple of higher level questions, and can do a secondary pass after discussions there.

[ghukill]

Have we tried this locally? I'm a little concerned that sys.exit(return_code) line in the sync CLI command may terminate the parent python process and therefore not make it past this step?

[ghukill]

Additionally and related, what happens if the sync subcommand has an error? how do we know and how is it handled?

[jonavellecuerdo]

(Tagging @ehanson8 for awareness)

This was a great question and led to the following changes:

• Add a try-except block in create CLI command
• Switching use of sys.exit to ctx.exit
  • As of click >= 8.2, ctx.exit raises a custom exception, click.exceptions.Exit, whereas before it would raise a SystemExit error. This was an important learning as it took me a second to figure out the right exception to handle with the added try-except block.
  • Not 100% sure of the differences but it is the recommended way to cleanly exit out of Click commands.
• Added unit test demonstrating that the create CLI command will cleanly exit when invoking the sync command raises an error

[ghukill]

Loving it! Thanks for the deep dive on this and the explanation.

[coveralls]

Pull Request Test Coverage Report for Build 18502412282

Details

• 33 of 34 (97.06%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.2%) to 96.015%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dsc/cli.py	28	29	96.55%

Files with Coverage Reduction	New Missed Lines	%
dsc/cli.py	1	88.89%

Totals
Change from base Build 18285846599:	0.2%
Covered Lines:	1036
Relevant Lines:	1079

---

💛 - Coveralls

[ehanson8]

Looks great to me, thanks for the changes!

[ehanson8]

Awesome!

[ghukill]

Looks good to me!