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

Update unity_sds_client with Data Service Delete #52

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brianlee731
Copy link
Contributor

@brianlee731 brianlee731 commented Jan 6, 2025

Purpose

  • Updated unity_sds_client to include the "delete_collection_data" function for Data Service.

Proposed Changes

  • Added "delete_collection_data" function for Data Service. Requires the collection object and granule_id.

Issues

Testing

Code for testing:
from unity_sds_client.unity import Unity
from unity_sds_client.unity_services import UnityServices
from unity_sds_client.unity import UnityEnvironments
from unity_sds_client.resources.collection import Collection
import requests

unity = Unity(UnityEnvironments.PROD)
token = unity._session.get_auth().get_token()
data_service = unity.client(UnityServices.DATA_SERVICE)

collections = data_service.get_collections(100)

granule_id = 'urn:nasa:unity:emit:dev:emit_ghg_test___1:emit20230809T105356_ch4_mf'

for i in range(len(collections)):
if 'urn:nasa:unity:emit:dev:emit_ghg_test___1' in collections[i].collection_id:
data_service.delete_collection_data(collections[i], granule_id)
break

@brianlee731 brianlee731 requested a review from mike-gangl January 6, 2025 00:29
granule_id = 'urn:nasa:unity:emit:dev:unity-tutorial___1:summary_table.txt'

for i in range(len(collections)):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not query for the specific collection we're going to delete from? getting a listing and then choosing the first (with no idea what it is?) is hardly a test. Does an error get thrown if we request a delete and the file doesn't exist?

For a test like this, i'd expect us to:

find/create a collection
add a file to it
check to see if that file exists
delete the file
ensure its deleted

delete the collection on cleanup. this is certainly an integration test, and i'm not sure we can do all of that right now (maybe talk to Nga) but how you're running this test is not going to be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

the delete you have is fine (but very manual), but the name of it is not great. When i saw it i thought it would delete all the data in the collection.

  1. call this "delete_collection_item" and keep the method as a colleciton_id + granule id
  2. Create another method that's called "delete_dataset" which takes as input a "dataset" object. The dataset objects we get back from a query to the system include the collection object and the granuleid string. This is more in keeping with the domain objects we've created.

Any methods created need to include the docstring that describes them. See https://github.com/unity-sds/unity-monorepo/blob/feature/unity-sds-client-update-ds-delete/libs/unity-py/unity_sds_client/resources/collection.py#L90-L99 as an example. without this peope won't really know how to use this method other than digging into the source code. Documentation ends up at https://unity-sds.github.io/unity-monorepo/unity-sds-client/

Both

Copy link
Contributor

@mike-gangl mike-gangl left a comment

Choose a reason for hiding this comment

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

See below comments. The testing is suspect and the delete methods should be clearer and more documented.

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