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

Handle lowercase daac and provider inputs #355

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Handle lowercase daac and provider inputs #355

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3b48f40
Handle lowercase daac and provider inputs
jrbourbeau Nov 15, 2023
f186096
Tests
jrbourbeau Nov 15, 2023
46814f7
Docstring
jrbourbeau Nov 16, 2023
880f45e
Link
jrbourbeau Nov 29, 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
16 changes: 12 additions & 4 deletions earthaccess/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@
from .utils import _validation as validate


def _normalize_location(location: Union[str, None]) -> Union[str, None]:
Copy link

Choose a reason for hiding this comment

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

Total nit that shouldn't be taken seriously: does Optional[str] seem like a readability improvement at all?

I'm writing Typescript lately where you can do location?: string and that's pretty slick.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Preface: I'm not very steeped in type annotations) To me Union seems more appropriate here because this is a required positional argument, not a keyword with a default of None. But I don't care strongly either way as long as linting checks pass.

Copy link

Choose a reason for hiding this comment

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

Union seems more appropriate here because this is a required positional argument, not a keyword with a default of None

Do you mean you think Union is more clear for that reason? Functionally they are the same thing, the argument would still be required: https://docs.python.org/3/library/typing.html#typing.Optional

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean you think Union is more clear for that reason?

Yes. Though I don't feel very strongly either way (or in general about type annotations). Happy to update if you'd prefer.

Copy link

Choose a reason for hiding this comment

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

I see the unclearness of Optional in that regard, good point! I also don't feel strongly, so let's leave it? :)

if location is not None:
location = location.upper()
return location


def search_datasets(
count: int = -1, **kwargs: Any
) -> List[earthaccess.results.DataCollection]:
Expand Down Expand Up @@ -170,6 +176,7 @@ def download(
Returns:
List of downloaded files
"""
provider = _normalize_location(provider)
if isinstance(granules, DataGranule):
granules = [granules]
try:
Expand All @@ -194,6 +201,7 @@ def open(
Returns:
a list of s3fs "file pointers" to s3 files.
"""
provider = _normalize_location(provider)
results = earthaccess.__store__.open(granules=granules, provider=provider)
return results

Expand All @@ -215,10 +223,8 @@ def get_s3_credentials(
Returns:
a dictionary with S3 credentials for the DAAC or provider
"""
if daac is not None:
daac = daac.upper()
if provider is not None:
provider = provider.upper()
daac = _normalize_location(daac)
provider = _normalize_location(provider)
if results is not None:
endpoint = results[0].get_s3_credentials_endpoint()
return earthaccess.__auth__.get_s3_credentials(endpoint=endpoint)
Expand Down Expand Up @@ -315,6 +321,8 @@ def get_s3fs_session(
Returns:
class s3fs.S3FileSystem: an authenticated s3fs session valid for 1 hour
"""
daac = _normalize_location(daac)
provider = _normalize_location(provider)
if results is not None:
endpoint = results[0].get_s3_credentials_endpoint()
if endpoint is not None:
Expand Down
22 changes: 22 additions & 0 deletions tests/integration/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import earthaccess
import pytest
import s3fs

logger = logging.getLogger(__name__)
assertions = unittest.TestCase("__init__")
Expand Down Expand Up @@ -94,3 +95,24 @@ def test_auth_can_fetch_s3_credentials():
print(
f"An error occured while trying to fetch S3 credentials for {daac['short-name']}: {e}"
)


@pytest.mark.parametrize("location", ({"daac": "podaac"}, {"provider": "pocloud"}))
def test_get_s3_credentials_lowercase_location(location):
activate_environment()
earthaccess.login(strategy="environment")
creds = earthaccess.get_s3_credentials(**location)
assert creds
assert all(
creds[key]
for key in ["accessKeyId", "secretAccessKey", "sessionToken", "expiration"]
)


@pytest.mark.parametrize("location", ({"daac": "podaac"}, {"provider": "pocloud"}))
def test_get_s3fs_session_lowercase_location(location):
activate_environment()
earthaccess.login(strategy="environment")
fs = earthaccess.get_s3fs_session(**location)
assert isinstance(fs, s3fs.S3FileSystem)
assert all(fs.storage_options[key] for key in ["key", "secret", "token"])
Loading