Refactored the loop in the tests by parameterizing #736

Sherwin-14 · 2024-06-30T09:05:45Z

I refactored the loop in here. I could only find this one to be refactored. What more could be done here?

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

tests/integration/test_auth.py

mfisher87 · 2024-06-30T14:30:26Z

tests/integration/test_auth.py

+        ("ASDC", "LARC_CLOUD"),
+    ),
+)
+def test_auth_can_fetch_s3_credentials(daac, provider):


We don't need provider here. We can also re-use the pre-existing earthaccess.daac.DAACS constant variable for the data structure instead!

Are you suggesting to use earthaccess.daac.DAACS as the variable from which we can access daac and providers, hence it cuts down the need of having a huge list of parameters ( the ones that I listed above)?

Correct, and also, the test never uses the provider variable, so it can be removed from the arguments and from the parameters in the decorator :)

tests/integration/test_auth.py

chuckwondo

@Sherwin-14, just a couple of suggestions to use standard assert instead of the unittest assert* methods, particularly since we're not using unittest for the function you've modified. Otherwise, your changes for parametrization look good.

tests/integration/test_auth.py

chuckwondo · 2024-07-24T14:26:24Z

@mfisher87, it looks like the same integration test failures are occurring for this PR as for #768. Do you have any insight on these since you've recently been trying to wrangle the integration tests?

mfisher87 · 2024-07-24T14:49:03Z

@Sherwin-14 have you set up secrets on your fork repository yet? When forking, secrets are not copied over. But integration tests rely on 2 secrets, EDL_USERNAME and EDL_PASSWORD:

      - name: Test
        env:
          EARTHDATA_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHDATA_PASSWORD: ${{ secrets.EDL_PASSWORD }}
          EARTHACCESS_TEST_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHACCESS_TEST_PASSWORD: ${{ secrets.EDL_PASSWORD }}
        run: poetry run bash scripts/integration-test.sh

You can add these through Settings > Secrets > Actions!

Related #773

Sherwin-14 · 2024-07-24T14:59:16Z

@Sherwin-14 have you set up secrets on your fork repository yet? When forking, secrets are not copied over. But integration tests rely on 2 secrets, EDL_USERNAME and EDL_PASSWORD:
      - name: Test
        env:
          EARTHDATA_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHDATA_PASSWORD: ${{ secrets.EDL_PASSWORD }}
          EARTHACCESS_TEST_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHACCESS_TEST_PASSWORD: ${{ secrets.EDL_PASSWORD }}
        run: poetry run bash scripts/integration-test.sh
You can add these through Settings > Secrets > Actions!

Related #773

Oh I didn't have any idea about this! This means we can now use this secrets to pass some of the integration tests?

chuckwondo · 2024-07-24T15:07:23Z

@Sherwin-14 have you set up secrets on your fork repository yet? When forking, secrets are not copied over. But integration tests rely on 2 secrets, EDL_USERNAME and EDL_PASSWORD:
      - name: Test
        env:
          EARTHDATA_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHDATA_PASSWORD: ${{ secrets.EDL_PASSWORD }}
          EARTHACCESS_TEST_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHACCESS_TEST_PASSWORD: ${{ secrets.EDL_PASSWORD }}
        run: poetry run bash scripts/integration-test.sh
You can add these through Settings > Secrets > Actions!

Related #773

Aha! That makes perfect sense now. I wasn't noticing that the failures are occurring against the fork even though the results show up in the main repo.

@Sherwin-14, each of the secret names EARTHDATA_USERNAME, EARTHDATA_PASSWORD, EARTHACCESS_TEST_USERNAME, and EARTHACCESS_TEST_PASSWORD, do the following for each:

go to https://github.com/Sherwin-14/earthaccess/settings/secrets/actions
click the New repository secret button
enter the secret name (one of the 4 names above) in the Name field
enter the appropriate value in the Secret field
click the Add secret button

The values for EARTHDATA_USERNAME and EARTHACCESS_TEST_USERNAME should be the same, and the values for EARTHDATA_PASSWORD and EARTHACCESS_TEST_PASSWORD should be the same, and these are the credentials for your account at https://urs.earthdata.nasa.gov/.

mfisher87 · 2024-07-24T15:14:49Z

I often get confused about forks, especially with the pull_request vs pull_request_target events behaving differently!

@chuckwondo I think there are only two secrets needed here actually, the ones on the right-hand side of the colon in the YAML. The ones on the left-hand side are what the GitHub secret names are being mapped to within the action step. I haven't tested this, but I'm fairly confident :)

chuckwondo · 2024-07-24T15:26:12Z

I often get confused about forks, especially with the pull_request vs pull_request_target events behaving differently!

@chuckwondo I think there are only two secrets needed here actually, the ones on the right-hand side of the colon in the YAML. The ones on the left-hand side are what the GitHub secret names are being mapped to within the action step. I haven't tested this, but I'm fairly confident :)

Ah, right, I read through that too quickly!

@Sherwin-14, regarding my previous instructions, just create the 2 secrets named EDL_USERNAME and EDL_PASSWORD, not the 4 secrets I mentioned above. These still represent the your credentials for your account at the URL I mentioned above.

Sherwin-14 · 2024-07-24T15:30:44Z

@Sherwin-14 have you set up secrets on your fork repository yet? When forking, secrets are not copied over. But integration tests rely on 2 secrets, EDL_USERNAME and EDL_PASSWORD:
      - name: Test
        env:
          EARTHDATA_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHDATA_PASSWORD: ${{ secrets.EDL_PASSWORD }}
          EARTHACCESS_TEST_USERNAME: ${{ secrets.EDL_USERNAME }}
          EARTHACCESS_TEST_PASSWORD: ${{ secrets.EDL_PASSWORD }}
        run: poetry run bash scripts/integration-test.sh
You can add these through Settings > Secrets > Actions!
Related #773
Aha! That makes perfect sense now. I wasn't noticing that the failures are occurring against the fork even though the results show up in the main repo.

@Sherwin-14, each of the secret names EARTHDATA_USERNAME, EARTHDATA_PASSWORD, EARTHACCESS_TEST_USERNAME, and EARTHACCESS_TEST_PASSWORD, do the following for each:
1. go to https://github.com/Sherwin-14/earthaccess/settings/secrets/actions

2. click the **New repository secret** button

3. enter the secret name (one of the 4 names above) in the **Name** field

4. enter the appropriate value in the **Secret** field

5. click the **Add secret** button
The values for EARTHDATA_USERNAME and EARTHACCESS_TEST_USERNAME should be the same, and the values for EARTHDATA_PASSWORD and EARTHACCESS_TEST_PASSWORD should be the same, and these are the credentials for your account at https://urs.earthdata.nasa.gov/.

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

chuckwondo · 2024-07-24T15:33:24Z

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

Sorry @Sherwin-14, I added another comment, as Matt corrected me on the secret names. Please see my new comment about the correct secret names.

Regarding resetting your password, please do that now because you'll need to set your password secret to your new password.

Sherwin-14 · 2024-07-24T15:43:39Z

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

Sorry @Sherwin-14, I added another comment, as Matt corrected me on the secret names. Please see my new comment about the correct secret names.

Regarding resetting your password, please do that now because you'll need to set your password secret to your new password.

Hey what I should choose as my affiliation? I am thinking of using category other

chuckwondo · 2024-07-24T15:57:09Z

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

Sorry @Sherwin-14, I added another comment, as Matt corrected me on the secret names. Please see my new comment about the correct secret names.
Regarding resetting your password, please do that now because you'll need to set your password secret to your new password.

Hey what I should choose as my affiliation? I am thinking of using category other

That's fine. I don't even remember what the choices are, but I don't think it really matters.

Sherwin-14 · 2024-07-24T16:03:56Z

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

Sorry @Sherwin-14, I added another comment, as Matt corrected me on the secret names. Please see my new comment about the correct secret names.
Regarding resetting your password, please do that now because you'll need to set your password secret to your new password.

Hey what I should choose as my affiliation? I am thinking of using category other

That's fine. I don't even remember what the choices are, but I don't think it really matters.

I have activated my account. What are the next steps form here?

chuckwondo · 2024-07-24T16:25:58Z

Hey I added the variables the account at https://urs.earthdata.nasa.gov/. The link is prompting me to reset my password should I leave this for now?

Sorry @Sherwin-14, I added another comment, as Matt corrected me on the secret names. Please see my new comment about the correct secret names.
Regarding resetting your password, please do that now because you'll need to set your password secret to your new password.

Hey what I should choose as my affiliation? I am thinking of using category other

That's fine. I don't even remember what the choices are, but I don't think it really matters.

I have activated my account. What are the next steps form here?

Just set your GitHub secrets (EDL_USERNAME and EDL_PASSWORD) as instructed in my other comment.

mfisher87 · 2024-07-24T16:50:57Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

chuckwondo · 2024-07-24T16:56:22Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

No need to do that. We can manually rerun the action for @Sherwin-14 once he let's us know that he has created his secrets.

Sherwin-14 · 2024-07-24T17:04:27Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

No need to do that. We can manually rerun the action for @Sherwin-14 once he let's us know that he has created his secrets.

Yeah I have created my secrets

chuckwondo · 2024-07-24T17:27:01Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

No need to do that. We can manually rerun the action for @Sherwin-14 once he let's us know that he has created his secrets.

Yeah I have created my secrets

Are you sure? I just re-ran the failed integration tests and they are still failing, and I'm still seeing this in the output:

INFO     test_api:test_api.py:17 Current username:

We should see the value of your secret named EDL_USERNAME at the end of the line above. Did you set EDL_USERNAME and EDL_PASSWORD in your repository secrets here: https://github.com/Sherwin-14/earthaccess/settings/secrets/actions

Sherwin-14 · 2024-07-24T17:53:45Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

No need to do that. We can manually rerun the action for @Sherwin-14 once he let's us know that he has created his secrets.

Yeah I have created my secrets

Are you sure? I just re-ran the failed integration tests and they are still failing, and I'm still seeing this in the output:
INFO     test_api:test_api.py:17 Current username: 
We should see the value of your secret named EDL_USERNAME at the end of the line above. Did you set EDL_USERNAME and EDL_PASSWORD in your repository secrets here: https://github.com/Sherwin-14/earthaccess/settings/secrets/actions

I have updated my secrets. Are the tests passing now?

chuckwondo · 2024-07-24T19:24:21Z

And then I believe you can close and re-open this pull request to trigger integration tests to re-run!

No need to do that. We can manually rerun the action for @Sherwin-14 once he let's us know that he has created his secrets.

Yeah I have created my secrets

Are you sure? I just re-ran the failed integration tests and they are still failing, and I'm still seeing this in the output:
INFO     test_api:test_api.py:17 Current username: 
We should see the value of your secret named EDL_USERNAME at the end of the line above. Did you set EDL_USERNAME and EDL_PASSWORD in your repository secrets here: https://github.com/Sherwin-14/earthaccess/settings/secrets/actions
I have updated my secrets. Are the tests passing now?

Nope. Still failing with a blank username. I think we need to get on a call so I can see what you're doing.

Sherwin-14 · 2024-07-25T11:00:58Z

I found something which might explain what is going on with the integration tests here https://github.com/orgs/community/discussions/26865

I believe the secrets that will be used in this case would be from the BASE repo and not my fork. so the secrets in my repo do not influence the behaviour of integration tests which are being run on BASE repo (which is this repo).

Sherwin-14 · 2024-07-25T11:09:06Z

@chuckwondo Here is my PR that I raised against my own repo which might support the above hypothesis Sherwin-14#1

The integration tests are now passing for certain versions and failing for some.

Update : Everything Passes now!

mfisher87 · 2024-07-25T14:39:31Z

Nice! This is a fairly complicated subject, so I wouldn't be surprised if I got something wrong. Apologies if so. That said, here's what I think I understand:

I think the thread you linked doesn't apply to our situation because it involves the issue_comment event type, which does always run against the repository in which the issue or PR was created (PRs are technically a type of issue). This is safe because creating a comment does not give the commenter power over the code! So they can't do something like steal a secret by making a comment.

For pull requests, there are two types, pull_request, which runs against the source repo to protect against secret exfiltration (consider a fork which creates a malicious action which emails secrets to themselves or prints the secrets in the logs -- then anyone could access any secret on GitHub just by forking if we allowed access to the source repository's secrets!!):

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request

..and pull_request_target, which runs against the target/base repository and critically disregards any code changes made in the pull request:

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

mfisher87 · 2024-07-25T14:42:02Z

I wonder if part of the reason is the method that's being used to trigger the workflow. When @chuckwondo triggers it manually, I think that counts as a workflow_dispatch event. I'm not sure how that is complicated by the involvement of a fork. If we want to trigger a pull_request action type correctly (i.e. on the fork), I think we need to close and re-open the PR (or push). I'm trying that now. Hopefully we learn something 😆

mfisher87 · 2024-07-25T14:46:46Z

Clearly that didn't work 😭 We're missing something important here. Sorry to say that at the moment I'm out of ideas 😬

mfisher87 · 2024-07-25T14:51:48Z

https://michaelheap.com/access-secrets-from-forks/

There are some tricks we can take from this. Also, we've discussed having a multi-branch release process that could remove the need for passing integration tests on PRs. But then it's harder for PRs like this, which change integration tests, to get live feedback.

Sherwin-14 · 2024-07-25T15:05:41Z

I see I missed that, however still, how would me adding a secret in my repository allow it on to work on these integration tests? I am a bit confused now.

…

On Thu, 25 Jul, 2024, 8:09 pm Matt Fisher, ***@***.***> wrote: Nice! This is a fairly complicated subject, so I wouldn't be surprised if I got something wrong. Apologies if so. That said, here's what I think I understand: I think the thread you linked doesn't apply to our situation because it involves the issue_comment event type, which does always run against the repository in which the issue or PR was created (PRs are technically a type of issue). This is safe because creating a comment does not give the commenter power over the code! So they can't do something like steal a secret by making a comment. For pull requests, there are two types, pull_request, which runs against the source repo to protect against secret exfiltration (consider a fork which creates a malicious action which emails secrets to themselves or prints the secrets in the logs -- then anyone could access any secret on GitHub just by forking if we allowed access to the source repository's secrets!!): https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request ..and pull_request_target, which runs against the target/base repository: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target — Reply to this email directly, view it on GitHub <#736 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/BBV63PYSP5HFZK3QHM2UBPDZOEE3RAVCNFSM6AAAAABKD5XZS2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJQGUYDANRTGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

mfisher87 · 2024-07-25T20:20:43Z

I am confused as well :) I think we may need to troubleshoot together on a call sometime soon! I will be on vacation next week, so the soonest I can make time is probably the next hack day or later.

Co-authored-by: Matt Fisher <mfisher87@gmail.com> Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

tests/integration/test_auth.py

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

chuckwondo

Looks good! I'm going to merge this one ahead of your other open PR because the same integration tests are failing for both, but his PR is smaller and has a much smaller impact.

Sherwin-14 · 2024-07-27T17:43:35Z

The tests did fail after merging as well, what is exactly going on here?

chuckwondo · 2024-07-27T18:00:56Z

I don't know where you're looking, but they all passed. See https://github.com/nsidc/earthaccess/actions/runs/10125275516

Refactored the loop in the tests by parameterzing the test

60c7f5a

Sherwin-14 force-pushed the testing branch from a4c2fe7 to 60c7f5a Compare June 30, 2024 09:08

Sherwin-14 changed the title ~~Refactored the loop in the tests by parameterzing the test~~ Refactored the loop in the tests by parameterzing Jun 30, 2024

Sherwin-14 changed the title ~~Refactored the loop in the tests by parameterzing~~ Refactored the loop in the tests by parameterizing Jun 30, 2024

mfisher87 reviewed Jun 30, 2024

View reviewed changes

tests/integration/test_auth.py Show resolved Hide resolved

mfisher87 reviewed Jun 30, 2024

View reviewed changes

mfisher87 mentioned this pull request Jul 2, 2024

Update integration tests to use pytest conventions #743

Closed

Removed provider and used the DAACS

84794f5

mfisher87 reviewed Jul 5, 2024

View reviewed changes

tests/integration/test_auth.py Outdated Show resolved Hide resolved

mfisher87 added the hackathon label Jul 8, 2024

chuckwondo requested changes Jul 24, 2024

View reviewed changes

tests/integration/test_auth.py Outdated Show resolved Hide resolved

tests/integration/test_auth.py Outdated Show resolved Hide resolved

mfisher87 mentioned this pull request Jul 24, 2024

Deprecated get_s3fs_session #768

Merged

mfisher87 closed this Jul 25, 2024

mfisher87 reopened this Jul 25, 2024

Sherwin-14 force-pushed the testing branch from bd040aa to c7aab14 Compare July 26, 2024 08:46

Apply suggestions from code review

d9663ac

Co-authored-by: Matt Fisher <mfisher87@gmail.com> Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

Sherwin-14 force-pushed the testing branch from eec5465 to d9663ac Compare July 26, 2024 08:53

chuckwondo requested changes Jul 26, 2024

View reviewed changes

tests/integration/test_auth.py Outdated Show resolved Hide resolved

tests/integration/test_auth.py Outdated Show resolved Hide resolved

Sherwin-14 and others added 2 commits July 27, 2024 15:48

Update assert tests/integration/test_auth.py

1005dc4

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

Remove logger info tests/integration/test_auth.py

873a7a4

Co-authored-by: Chuck Daniels <cjdaniels4@gmail.com>

Sherwin-14 force-pushed the testing branch from 316e0d1 to 873a7a4 Compare July 27, 2024 10:26

Sherwin-14 requested review from chuckwondo and mfisher87 July 27, 2024 12:45

chuckwondo approved these changes Jul 27, 2024

View reviewed changes

chuckwondo merged commit 3a8f6ac into nsidc:main Jul 27, 2024
7 of 11 checks passed

chuckwondo mentioned this pull request Jul 27, 2024

Refactor loops in tests to be parametrized #578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored the loop in the tests by parameterizing #736

Refactored the loop in the tests by parameterizing #736

Sherwin-14 commented Jun 30, 2024 •

edited by github-actions bot

Loading

mfisher87 Jun 30, 2024

Sherwin-14 Jun 30, 2024

mfisher87 Jun 30, 2024

chuckwondo left a comment

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024 •

edited

Loading

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024 •

edited

Loading

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024 •

edited

Loading

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 25, 2024 •

edited

Loading

Sherwin-14 commented Jul 25, 2024 •

edited

Loading

mfisher87 commented Jul 25, 2024 •

edited

Loading

mfisher87 commented Jul 25, 2024 •

edited

Loading

mfisher87 commented Jul 25, 2024

mfisher87 commented Jul 25, 2024

Sherwin-14 commented Jul 25, 2024 via email

mfisher87 commented Jul 25, 2024

chuckwondo left a comment

Sherwin-14 commented Jul 27, 2024

chuckwondo commented Jul 27, 2024

Refactored the loop in the tests by parameterizing #736

Refactored the loop in the tests by parameterizing #736

Conversation

Sherwin-14 commented Jun 30, 2024 • edited by github-actions bot Loading

mfisher87 Jun 30, 2024

Choose a reason for hiding this comment

Sherwin-14 Jun 30, 2024

Choose a reason for hiding this comment

mfisher87 Jun 30, 2024

Choose a reason for hiding this comment

chuckwondo left a comment

Choose a reason for hiding this comment

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024 • edited Loading

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024 • edited Loading

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

mfisher87 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 24, 2024 • edited Loading

chuckwondo commented Jul 24, 2024

Sherwin-14 commented Jul 25, 2024 • edited Loading

Sherwin-14 commented Jul 25, 2024 • edited Loading

mfisher87 commented Jul 25, 2024 • edited Loading

mfisher87 commented Jul 25, 2024 • edited Loading

mfisher87 commented Jul 25, 2024

mfisher87 commented Jul 25, 2024

Sherwin-14 commented Jul 25, 2024 via email

mfisher87 commented Jul 25, 2024

chuckwondo left a comment

Choose a reason for hiding this comment

Sherwin-14 commented Jul 27, 2024

chuckwondo commented Jul 27, 2024

Sherwin-14 commented Jun 30, 2024 •

edited by github-actions bot

Loading

mfisher87 commented Jul 24, 2024 •

edited

Loading

Sherwin-14 commented Jul 24, 2024 •

edited

Loading

Sherwin-14 commented Jul 24, 2024 •

edited

Loading

Sherwin-14 commented Jul 25, 2024 •

edited

Loading

Sherwin-14 commented Jul 25, 2024 •

edited

Loading

mfisher87 commented Jul 25, 2024 •

edited

Loading

mfisher87 commented Jul 25, 2024 •

edited

Loading