Unable to receive zipped deliveries to GCP

[kevinlacaille]

Expected behavior
Given this style cloudconfig.json file, as specified in order_request (here) and validated in specs (here) I expect to be able to receive a zipped file delivered my GCS bucket.

{
    "google_cloud_storage": {
        "bucket": "pfed_external_share",
        "path_prefix": "zip_test",
        "credentials": "my-creds"
    },
    "archive_type": "zip"
}

Untested, but this might be a global issue (i.e., an issue with AWS, GCS, Azure, etc.).

Actual behavior (describe the problem)
My order is not delivered zipped.

Related Issues
N/A

Workaround
N/A

Minimum, Complete, Viable Code Sample
Apparently, one must also set a "archive_filename" (e.g. "archive_filename": "{{name}}_{{order_id}}.zip" ) or use "single_archive": true to actually get zipped deliveries.

{
    "google_cloud_storage": {
        "bucket": "pfed_external_share",
        "path_prefix": "zip_test",
        "credentials": "my-creds"
    },
    "archive_type": "zip",
    "archive_filename": "{{name}}_{{order_id}}.zip"
}

or

{
    "google_cloud_storage": {
        "bucket": "pfed_external_share",
        "path_prefix": "zip_test",
        "credentials": "my-creds"
    },
    "archive_type": "zip",
    "single_archive": true
}

[jreiberkyle]

Yep, looks like we need some more logic added to delivery for handling zipping.

[JSee98]

Hi @kevinlacaille, this seems like a good first-time issue to me. I looked at the following function that updates the download config with the archive file name and other related details

planet-client-python/planet/order_request.py

Lines 165 to 200 in 79d9a3c

	def delivery(archive_type: Optional[str] = None,
	single_archive: bool = False,
	archive_filename: Optional[str] = None,
	cloud_config: Optional[dict] = None) -> dict:
	"""Order delivery configuration.
	Example:
	```python
	amazon_s3_config = amazon_s3(
	'access_key',
	'secret_access_key',
	'bucket_name',
	'us-east-2',
	'folder1/prefix/'
	)
	delivery_config = delivery(
	archive_type='zip',
	single_archive=True,
	archive_filename='{{order_id}}.zip'
	cloud_config=amazon_s3_config
	)
	```
	Parameters:
	archive_type: Archive order files. Only supports 'zip'.
	single_archive: Archive all bundles together in a single file.
	archive_filename: Custom naming convention to use to name the
	archive file that is received. Uses the template variables
	{{name}} and {{order_id}}. e.g. "{{name}}_{{order_id}}.zip".
	cloud_config: Cloud delivery configuration.
	Raises:
	planet.specs.SpecificationException: If archive_type is not valid.
	"""
	if archive_type:
	archive_type = specs.validate_archive_type(archive_type)

The issue seems to be the fact that archive_name and single_archive are optional fields and a check isn't performed, specifically, "archive_type": "zip" is defined and the following two fields are undefined, thus, causing this issue.

Let me know if I can pick this up.

[kevinlacaille]

Hi, @JSee98! Glad to see you back at it here :)

You're right, the logic around archive_type is probably the root of the problem here. I'm not 100% sure, but I believe that if we set archive_type to true, then either we need to set archive_filename (e.g. "archive_filename": "{{name}}_{{order_id}}.zip" ) or set "single_archive": true.

Please feel free to pick up this ticket and I will happily guide you and review your PR.

[JSee98]

Thanks @kevinlacaille will pick this up. Got a relatively simple idea in my mind.

If archive_type is defined, then three outcomes are possible:

1. Archive file name is given (currently works)
2. single archive flag is given (currently works)
3. If neither of those fields are defined then we can either throw an error to ask the user to define either of the two flows, or to maintain backward compatibility, just set single-archive flag to true and let the existing code to take over.

I hope I make sense here.

If yes, I would need a go ahead on one of these approaches (personally I prefer the one with backwards compatibility, i.e. set single-archive to true)

[kevinlacaille]

@JSee98 I think I prefer your second approach:

If neither of those fields are defined ... set single-archive flag to true and let the existing code to take over

That seems like a simple and elegant solution and would require little to no work for the tests.

[JSee98]

Hi @kevinlacaille, raised the PR for the change. Added unit tests as well. Need a little help with nox. Tried running it locally, however, it seems some files that I haven't changed are failing linting. Will share the output if needed.

[JSee98]

Thanks, @kevinlacaille and @sgillies.
Fixing the first issue was fun. Hope to keep this going.
Already have eyes on new issues.