IMAP-Science-Operations-Center / imap-data-access

Package to download, query, and upload files from the IMAP Science Data Center.
MIT License
0 stars 7 forks source link

FIX: Remove restriction on upload data directory structure #43

Closed greglucas closed 3 weeks ago

greglucas commented 3 weeks ago

Change Summary

Overview

Our APIs don't require the full path anymore, they can use just the filename and infer the directory structure. This is helpful for the upload to be able to upload a file from the current working directory.

imap-data-access -v upload imap_mag_l1a_burst-magi_20240314_v007.cdf 
INFO:imap_data_access.io:Uploading file /Users/grlu5547/code/imap/imap-data-access/data/imap/mag/l1a/2024/03/imap_mag_l1a_burst-magi_20240314_v007.cdf to https://api.dev.imap-mission.com/upload/imap_mag_l1a_burst-magi_20240314_v007.cdf
HTTP Error: 409 - Conflict
Server Message: "imap/mag/l1a/2024/03/imap_mag_l1a_burst-magi_20240314_v007.cdf already exists."

closes #41

tech3371 commented 3 weeks ago

I was trying to find doc strings to update which reflects this change. Looks like there are few places to update. Mainly in this README.md file and this function doc string

greglucas commented 3 weeks ago

I was trying to find doc strings to update which reflects this change. Looks like there are few places to update. I can help pin down which ones needs update.

I didn't find any that needed to be updated, so if you have ones you think should be changed then yes please let me know. I thought all of the other locations were valid still. For example: https://github.com/IMAP-Science-Operations-Center/imap-data-access/blob/82e9bbb1b51c30a41c10af6d6892747ded89e0de/imap_data_access/cli.py#L169-L170 is still correct because you must give the path to the file, we don't infer it anywhere. Maybe changing it to "This can be a relative or absolute path." would help though?

greglucas commented 3 weeks ago

@tech3371 I didn't actually remove the default directory data/ requirement on anything other than uploading. So, all path validation and setting of the IMAP_DATA_ACCESS global variables are still valid I think. The only thing this PR does is remove the check that we are within that structure. It lets you upload relative paths. imap-data-access path/to/file.txt is equivalent to imap-data-access $PWD/path/to/file.txt now and doesn't care that IMAP_DATA_ACCESS directory is pointing to /my/other/location.

vmartinez-cu commented 3 weeks ago

I was trying to find doc strings to update which reflects this change. Looks like there are few places to update. I can help pin down which ones needs update.

I didn't find any that needed to be updated, so if you have ones you think should be changed then yes please let me know. I thought all of the other locations were valid still. For example:

https://github.com/IMAP-Science-Operations-Center/imap-data-access/blob/82e9bbb1b51c30a41c10af6d6892747ded89e0de/imap_data_access/cli.py#L169-L170

is still correct because you must give the path to the file, we don't infer it anywhere. Maybe changing it to "This can be a relative or absolute path." would help though?

The README has the following statement regarding the data directory. Does this only apply to downloading files and not to uploading files?

If the ``IMAP_DATA_DIR`` variable is not set, the program defaults to the user's current working directory + ``data/``.

greglucas commented 3 weeks ago

The README has the following statement regarding the data directory. Does this only apply to downloading files and not to uploading files?

If the IMAP_DATA_DIR variable is not set, the program defaults to the user's current working directory + data/.

Yes, that is correct with this PR. Would it help to add something like this sentence?

The IMAP_DATA_DIR is only used for downloads.

vmartinez-cu commented 3 weeks ago

Yes, that is correct with this PR. Would it help to add something like this sentence?

The IMAP_DATA_DIR is only used for downloads.

Okay thanks for the clarification. Yeah, I think a short note about that would be helpful

greglucas commented 3 weeks ago

Merging now, feel free to open another PR or leave suggestions here and I'm happy to follow-up if there are any other docs suggestions.