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

FIX: Remove restriction on upload data directory structure #43

Conversation

greglucas
Copy link
Contributor

@greglucas greglucas commented Jun 10, 2024

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

Copy link
Contributor

@tech3371 tech3371 left a comment

Choose a reason for hiding this comment

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

LGTM!

@tech3371
Copy link
Contributor

tech3371 commented Jun 10, 2024

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
Copy link
Contributor Author

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:

"Upload a file to the IMAP SDC. This must be the full path to the file."
"\nE.g. imap/mag/l0/2025/01/imap_mag_l0_raw_20250101_v001.pkts"

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
Copy link
Contributor Author

@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
Copy link

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:

"Upload a file to the IMAP SDC. This must be the full path to the file."
"\nE.g. imap/mag/l0/2025/01/imap_mag_l0_raw_20250101_v001.pkts"

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
Copy link
Contributor Author

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
Copy link

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 greglucas force-pushed the rm-upload-restrictions branch from 0908a12 to 5846db5 Compare June 10, 2024 21:36
@greglucas
Copy link
Contributor Author

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.

@greglucas greglucas merged commit 2a1abc9 into IMAP-Science-Operations-Center:main Jun 11, 2024
13 checks passed
@greglucas greglucas deleted the rm-upload-restrictions branch June 11, 2024 01:57
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.

Remove the requirement to upload from a specific data folder structure
3 participants