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.

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

Jump to bottom

Updated order delivery config for zipped orders #989

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Updated order delivery config for zipped orders #989

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4bf7675
zipped-order-delivery-963: Updated delivery function in orders
JSee98 Jul 12, 2023
a5dfa0e
zipped-order-delivery-963: updated unit tests
JSee98 Jul 13, 2023
8a0e672
zipped-order-delivery-963: updated order request logic
JSee98 Jul 16, 2023
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
1 change: 0 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,5 @@ coverage.xml

# Editors
.vscode/

# Docs build
site
4 changes: 4 additions & 0 deletions planet/order_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ def delivery(archive_type: Optional[str] = None,
if archive_type:
archive_type = specs.validate_archive_type(archive_type)

# for missing archive options set single_archive to false
if archive_filename is None and single_archive is False:
kevinlacaille marked this conversation as resolved.
Show resolved Hide resolved
single_archive = True
Copy link
Contributor

Choose a reason for hiding this comment

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

@JSee98 @kevinlacaille I have a different perspective on the problem. I don't think we want to change the single/multi flag, and thus the shape of the delivered artifacts, based on other inputs. Instead I think we want to make sure that the archive_filename defaults to some value if it is not specified. After a close read of https://developers.planet.com/apis/orders/delivery/#zipping-results, I believe we want something like this:

if archive_type:
    archive_filename = archive_filename or "{{name}}_{{order_id}}.zip"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So just to confirm @sgillies, your suggestion is if both the variables are undefined i.e. archive_filename and single_archive we should give a default value to archive_filename?
If you and @kevinlacaille agree on this suggested path I can make the required changes.

Copy link
Contributor

@sgillies sgillies Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@JSee98 we should provide a default for archive_filename whenever archive_type evaluates to True, no matter the value of single_archive. @kevinlacaille (traveling at the moment, so I'm speaking for him 😆) asked the API team and found that the need to specify an archive filename is a known Orders API bug and that this Python project is the right place to work around it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure guys, will make the updates and raise the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgillies I've made the updates


fields = ['archive_type', 'single_archive', 'archive_filename']
values = [archive_type, single_archive, archive_filename]

Expand Down
25 changes: 25 additions & 0 deletions tests/unit/test_order_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,31 @@ def test_delivery():
assert delivery_config == expected


def test_delivery_missing_archive_details():
as3_config = {
'amazon_s3': {
'aws_access_key_id': 'aws_access_key_id',
'aws_secret_access_key': 'aws_secret_access_key',
'bucket': 'bucket',
'aws_region': 'aws_region'
}
}
delivery_config = order_request.delivery(archive_type='zip',
cloud_config=as3_config)

expected = {
'archive_type': 'zip',
'single_archive': True,
'amazon_s3': {
'aws_access_key_id': 'aws_access_key_id',
'aws_secret_access_key': 'aws_secret_access_key',
'bucket': 'bucket',
'aws_region': 'aws_region'
}
}
assert delivery_config == expected


def test_amazon_s3():
as3_config = order_request.amazon_s3('aws_access_key_id',
'aws_secret_access_key',
Expand Down
Loading