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

Create dest dir for direct S3 access before download #570

Merged
merged 1 commit into from
May 13, 2024

Conversation

chuckwondo
Copy link
Collaborator

@chuckwondo chuckwondo commented May 13, 2024

I didn't add a unit or integration test for this, as I wasn't sure about how best to test this, so I'm open to suggestion.

Fixes #562


📚 Documentation preview 📚: https://earthaccess--570.org.readthedocs.build/en/570/

@chuckwondo chuckwondo requested a review from mfisher87 May 13, 2024 14:35
@chuckwondo chuckwondo changed the title Create dest dir for direct access before download Create dest dir for direct S3 access before download May 13, 2024
Copy link
Collaborator

@mfisher87 mfisher87 left a comment

Choose a reason for hiding this comment

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

Looks good so far! For testing, I'm thinking we could extract a download_s3_granules(path: Path, data_links: list[str]) -> list[Path] function and give it the input required to reproduce the bug in a test?

Really we should be testing that any time we write granules we will correctly create an output directory, but if nobody has time to do that refactoring right now, the above approach would at least add coverage to this change 🤷 No strong feelings.

@@ -6,6 +6,9 @@
* Bug fixes:
* fixed 483 by extracting a common CMR query method for collections and granules using SearchAfter header
* Added VCR support for verifying the API call to CMR and the parsing of returned results without relying on CMR availability post development
* [#562](https://github.com/nsidc/earthaccess/issues/562): The destination
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indentation is off by 2 spaces with the items above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! I'll sort that out with my other PR, where I've reformatted the section. I didn't want to create much of a merge conflict for myself here.

@chuckwondo
Copy link
Collaborator Author

Looks good so far! For testing, I'm thinking we could extract a download_s3_granules(path: Path, data_links: list[str]) -> list[Path] function and give it the input required to reproduce the bug in a test?

Really we should be testing that any time we write granules we will correctly create an output directory, but if nobody has time to do that refactoring right now, the above approach would at least add coverage to this change 🤷 No strong feelings.

I think the problem with testing this is that we cannot guarantee that tests will run in an AWS environment that's "in region." If that's not possible, then manually setting earthaccess.__store__.in_region = True doesn't help us because if the test is not actually running "in region", attempting a direct S3 download simply results in PermissionError: Access Denied.

@mfisher87
Copy link
Collaborator

Hm. I don't have any great ideas here. @betolink ?

I'd be fine with merging without adding a test and recording a bug to add a test later. Better to get the fix in the hands of users sooner IMO. We can flail around figuring out unit tests on our time 🤣

@chuckwondo chuckwondo merged commit 3a0fb8f into nsidc:main May 13, 2024
11 checks passed
@chuckwondo chuckwondo deleted the issue562-fsspec-destdir branch May 13, 2024 17:32
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.

download while on AWS creates one file
2 participants