Issue 421 - option to use Earthdata User Acceptance Testing (UAT) system

[danielfromearth]

GitHub Issue: #421

Description

This change:

• enables the login and use of different Earthdata Login (EDL) and Common Metadata Repository (CMR) systems — that is, either Production (PROD) or User Acceptance Testing (UAT).
• removes the Auth.get_user_profile() method and use of a client_id.

Local test steps

Passed new UAT unit test

Overview of integration done

Ran the following (with an ID for a TEMPO mission collection in UAT substituted for <UAT Collection ID>):

import earthaccess
auth = earthaccess.login(system=earthaccess.UAT)
results = earthaccess.search_data(concept_id=<UAT Collection ID>)
files = earthaccess.download(results, "./local_folder")

...and this generated an appropriate response, and successfully download the granule files.

Checklist

• Unit tests added/updated and passing.
• Integration testing
• CHANGELOG.md updated
• Documentation updated (if needed).

---

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

[mfisher87]

Daniel and I discussed integration testing strategy here. Since UAT tests could be more flaky than PROD tests, we decided we will write only unit tests for this PR (plus one sample integration test) and address the problem of flaky tests in another effort. I'll link the new issue to this one.

[danielfromearth]

And with regard to unit tests, we discussed creating a new module which holds all UAT unit tests in the test suite, instead of duplicating+modifying the existing tests within the existing modules.

[mfisher87]

@danielfromearth @chuckwondo sorry for the delay pushing this up. I just forgot 😊

[chuckwondo]

@chuckwondo

Further, I think we may want to drop SIT, and offer only PROD and UAT.

That's fine with me. I am only away of SIT use-cases currently.

@danielfromearth, just to confirm my suspicion about needing a separate client ID per system/env, did you attempt to test a scenario against UAT where a client ID would be required? I would think that such a test would fail if you were to supply the PROD client ID we currently have hard-coded.

On this branch, I successfully ran earthaccess's login(), search_data(), and download() for a collection in UAT. It found the granules and downloaded the files. So, perhaps this client ID is associated with both "systems"?

I'm not sure that's a valid test of the client ID, as I believe your unit tests simply mock responses, but please correct me if I'm wrong (I haven't looked at all of your relevant unit tests to confirm this). If that's the case, then you may need to make use of vcrpy to make (and record) real requests/responses to see if tests requiring a client ID fail when making a real request to UAT with what I suspect is a PROD-only client ID.

[chuckwondo]

@chuckwondo

Further, I think we may want to drop SIT, and offer only PROD and UAT.

That's fine with me. I am only away of SIT use-cases currently.

@danielfromearth, just to confirm my suspicion about needing a separate client ID per system/env, did you attempt to test a scenario against UAT where a client ID would be required? I would think that such a test would fail if you were to supply the PROD client ID we currently have hard-coded.

On this branch, I successfully ran earthaccess's login(), search_data(), and download() for a collection in UAT. It found the granules and downloaded the files. So, perhaps this client ID is associated with both "systems"?

I'm not sure that's a valid test of the client ID, as I believe your unit tests simply mock responses, but please correct me if I'm wrong (I haven't looked at all of your relevant unit tests to confirm this). If that's the case, then you may need to make use of vcrpy to make (and record) real requests/responses to see if tests requiring a client ID fail when making a real request to UAT with what I suspect is a PROD-only client ID.

Upon further inspection, I'm inclined to believe that we can completely eliminate the client ID. What makes me believe this is that, on the main branch, I ran all integration tests, and I got 9 failures, which I believe are to be expected when running locally (I recall this from a previous topic of discussion during a "hack day"). I then removed all uses of client ID throughout the entire codebase, reran the integration tests, and got identical results.

There are many tests that download files, and all succeeded without the client ID (other than the ones that also failed with the client ID).

I believe the only case for requiring a client ID is for a web app that requires redirection after login. For EDL to know where to redirect the user, you must supply a client ID and the registered app with that client ID must be configured with a redirect URL. This is not the case for earthaccess, so as far as I can tell, we should be able to completely drop the use of a client ID.

However, since I'm not completely confident in my line of thinking here (even though test results seem to point in this direction), I have written email to [email protected] to get a definitive answer for us.

[danielfromearth]

@chuckwondo

Further, I think we may want to drop SIT, and offer only PROD and UAT.

That's fine with me. I am only away of SIT use-cases currently.

@danielfromearth, just to confirm my suspicion about needing a separate client ID per system/env, did you attempt to test a scenario against UAT where a client ID would be required? I would think that such a test would fail if you were to supply the PROD client ID we currently have hard-coded.

On this branch, I successfully ran earthaccess's login(), search_data(), and download() for a collection in UAT. It found the granules and downloaded the files. So, perhaps this client ID is associated with both "systems"?

I'm not sure that's a valid test of the client ID, as I believe your unit tests simply mock responses, but please correct me if I'm wrong (I haven't looked at all of your relevant unit tests to confirm this). If that's the case, then you may need to make use of vcrpy to make (and record) real requests/responses to see if tests requiring a client ID fail when making a real request to UAT with what I suspect is a PROD-only client ID.

Right, the unit tests mock responses. What I ran for this test was not a unit test; instead, I manually ran earthaccess in a Python kernel to login and download files — for my case, TEMPO data granules — from UAT. And then manually confirmed that the downloaded files were the expected files.

[danielfromearth]

@mfisher87, @chuckwondo, would you be able to help address the new readthedocs failures that have cropped up after merging main?

[chuckwondo]

@mfisher87, @chuckwondo, would you be able to help address the new readthedocs failures that have cropped up after merging main?

@danielfromearth, take a look at the details of the failure: https://readthedocs.org/projects/earthaccess/builds/24322214/

You'll see this in the error output:

WARNING -  griffe: earthaccess/auth.py:251: No type or annotation for parameter 'system'
WARNING -  griffe: earthaccess/auth.py:251: Parameter 'system' does not appear in the function signature

Given that, here's what I see in the code in your branch that would cause that warning:

earthaccess/earthaccess/auth.py

Lines 246 to 251 in aeaa280

	def get_session(self, bearer_token: bool = True) -> requests.Session:
	"""Returns a new request session instance.
	Parameters:
	bearer_token: whether to include bearer token
	system: optional EARTHDATA environment to use

Can you see the issue? In your docstring, you document a system parameter to the get_session method, but there is no such parameter for that method. Should there be, or should you remove the entry from the docstring?

[danielfromearth]

Thanks @chuckwondo for identifying the change needed! I was overthinking it, and didn't realize the other messages were simply "INFO" messages and not "warnings" 🤦

[danielfromearth]

Do we want to remove SIT?

[chuckwondo]

Do we want to remove SIT?

I suggest we do.

Upon further inspection, I'm inclined to believe that we can completely eliminate the client ID. What makes me believe this is that, on the main branch, I ran all integration tests, and I got 9 failures, which I believe are to be expected when running locally (I recall this from a previous topic of discussion during a "hack day"). I then removed all uses of client ID throughout the entire codebase, reran the integration tests, and got identical results.

There are many tests that download files, and all succeeded without the client ID (other than the ones that also failed with the client ID).

I believe the only case for requiring a client ID is for a web app that requires redirection after login. For EDL to know where to redirect the user, you must supply a client ID and the registered app with that client ID must be configured with a redirect URL. This is not the case for earthaccess, so as far as I can tell, we should be able to completely drop the use of a client ID.

However, since I'm not completely confident in my line of thinking here (even though test results seem to point in this direction), I have written email to [email protected] to get a definitive answer for us.

Further, I think we should wait for me to get a reply from [email protected] regarding the client ID. Ideally, my hunch is correct, and we can simply remove all uses of the client ID. I did get an automated reply, so hopefully I'll get an answer this week. Sorry to hold this up further, but I'm not comfortable with leaving the questions surrounding the client ID unanswered.

[mfisher87]

@chuckwondo could you also start a discussion on Slack about the client ID? I think Luis likely knows more but has limited time to catch up on this PR. I am fairly sure you're right that we don't need it. I vaguely remember discussions of using it for metrics purposes, but I think I'm confusing that with the user agent string.

Nevermind I see you've looped him in to the related issue :)

[chuckwondo]

Fantastic! I have only a handful of very minor grammatical suggestions (mostly around consistent "system" wording), and a couple of very minor code tweaks.

[chuckwondo]

Well done @danielfromearth!

[danielfromearth]

Thank you @chuckwondo for your close look at all of the changes in this PR!