Allow maintainers to run integration tests on PR from a fork #818

chuckwondo · 2024-09-18T23:12:50Z

This PR fixes the problem of failing integration tests on PRs from forked repos by allowing maintainers to review such PRs for potential security issues (which should be minimal for trusted/known contributors) and then re-running the failed jobs such that they may successfully run.

The result is the following:

In the integration test job, the permissions of the user (actor) triggering the PR are checked. If the user does not have write access to this repo, the job is immediately failed, thus avoiding any attempt to run integration tests, which would only fail anyway because without write permission, the secrets required by the integration tests cannot be read.
The job summary for the failed integration tests describe the problem (i.e., the triggering user does not have permission to run int. tests, and a maintainer must perform a security review of the PR, and re-run the failed jobs if there are no security concerns).
After a maintainer determines there are no security concerns, the maintainer can re-run the failed build, which will allow the integration jobs to run with the necessary permission to read secrets so that the integration tests may succeed.
Once the integration tests succeed, there is no longer a need for a maintainer to bypass failing integration tests in the hope that they will pass once the PR is merged.

Since this change must land on the main branch before it takes effect, I tested this with a fork from my fork to confirm that it does indeed work as expected. This should mean that this PR should be the last PR a maintainer will have to bypass failing integration tests to merge it.

Pull Request (PR) draft checklist - click to expand

Please review our
contributing documentation
before getting started.
Ensure an issue exists representing the problem being solved in this PR.
Populate a descriptive title. For example, instead of "Updated README.md", use a
title such as "Add testing details to the contributor section of the README".
Populate the body of the pull request with:
- A clear description of the change you are proposing.
- Links to any issues resolved by this PR with text in the PR description, for
  example closes #1. See
  GitHub docs - Linking a pull request to an issue.
Update CHANGELOG.md with details about your change in a section titled
## Unreleased. If such a section does not exist, please create one. Follow
Common Changelog for your additions.
Update the documentation and/or the README.md with details of changes to the
earthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.

Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!

Pull Request (PR) merge checklist - click to expand

Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the @nsidc/earthaccess-support team in a comment and we
will help you out!

Add unit tests for any new features.
Apply formatting and linting autofixes. You can add a GitHub comment in this Pull
Request containing "pre-commit.ci autofix" to automate this.
Ensure all automated PR checks (seen at the bottom of the "conversation" tab) pass.
Get at least one approving review.

Fixes #810

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

If a user has either the `PIP_REQUIRE_VENV` or `PIP_REQUIRE_VIRTUALENV` environment variable set to a "truthy" value, and no virtual environment is active, `pip install` will fail with a message indicating as such. However, when a conda environment is active, we want to allow `pip install` to run without an active virtual environment because it will install packages within the active conda environment.

Fixes nsidc#810

codecov-commenter · 2024-09-18T23:14:46Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@bb553e1). Learn more about missing BASE report.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #818   +/-   ##
=======================================
  Coverage        ?   68.00%           
=======================================
  Files           ?       22           
  Lines           ?     1466           
  Branches        ?        0           
=======================================
  Hits            ?      997           
  Misses          ?      469           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chuckwondo · 2024-09-18T23:18:10Z

Looks like pull_request integration tests were properly skipped, but that pull_request_target integration tests were not even triggered. However, I believe this is to be expected because the pull_request_target was added by this PR, so won't be triggered until after landing on the main branch. Fortunately, since there are no code changes in the PR, there should be no concern about integration tests not running for this PR.

chuckwondo · 2024-09-20T15:44:11Z

@mfisher87, @betolink, or @jhkennedy, any chance of getting eyes on this so we can resolve the integration tests problem? It's a rather small PR.

betolink · 2024-09-20T15:46:15Z

I can take a look now @chuckwondo

betolink

I like it, I think we can merge it and start testing it!

betolink · 2024-09-20T15:47:40Z

environment.yml

@@ -9,3 +9,7 @@ dependencies:
  - pip
  - pip:
      - --editable .[dev,test,docs]
+variables:


Nice, I didn't know about this

betolink · 2024-09-20T15:53:20Z

noxfile.py

@@ -8,7 +8,7 @@
 DIR = Path(__file__).parent.resolve()

 nox.needs_version = ">=2024.3.2"
-nox.options.sessions = ["typecheck", "test_unit"]
+nox.options.sessions = ["typecheck", "tests"]


Maybe we need to add a comment in the docs that for now nox tests will only run unit tests?

This actually better matches my expectations -- I generally would expect pytest/nox to only run unit tests unless explicitly told to run integration tests as they are larger and require integrations. It's pretty common for unit tests to just be called "tests".

@betolink, we can clarify the docs when we add integration tests to the mix with nox as per the recent issue you opened.

@jhkennedy, yep, agreed.

Perhaps the yet-to-be-added nox session could be named integration_tests (or integration-tests, if nox supports kebab-case names -- not sure if it does).

jhkennedy

This looks good to me!

jhkennedy · 2024-09-20T16:28:55Z

noxfile.py

@@ -8,7 +8,7 @@
 DIR = Path(__file__).parent.resolve()

 nox.needs_version = ">=2024.3.2"
-nox.options.sessions = ["typecheck", "test_unit"]
+nox.options.sessions = ["typecheck", "tests"]


This actually better matches my expectations -- I generally would expect pytest/nox to only run unit tests unless explicitly told to run integration tests as they are larger and require integrations. It's pretty common for unit tests to just be called "tests".

chuckwondo · 2024-09-20T17:51:51Z

Thanks @betolink and @jhkennedy. Here goes nothin'!

mfisher87 · 2024-09-21T02:50:37Z

.github/workflows/integration-test.yml

+        run: |
+          echo "User **${{ github.triggering_actor }}** does not have permission to run integration tests." >> $GITHUB_STEP_SUMMARY
+          echo "A maintainer must perform a security review and re-run this build, if the code is safe." >> $GITHUB_STEP_SUMMARY
+          echo "See [Keeping your GitHub Actions and workflows secure Part 1: Preventing pwn requests](https://securitylab.github.com/resources/github-actions-preventing-pwn-requests)." >> $GITHUB_STEP_SUMMARY


@chuckwondo I could have sworn i commented on this and a couple other things, but I don't see those comments anywhere! Can you help me out? I was thinking we could link directly to the build log for the job that needs to be re-run.

Hey @mfisher87, sorry for the confusion. I think your comment was on my duplicate PR from a branch within the earthaccess repo instead of from my fork. I had initially created that other draft PR to make sure things would work for a PR within the repo, but once confirmed, closed it in favor of this one from my fork, as this is the other case we need to cover.

Regarding your "missing" comment, I tweaked your suggestion (from the other PR) here in 2 ways:

I replaced GITHUB_OUTPUT with GITHUB_STEP_SUMMARY, as I believe this is perhaps what you intended (GITHUB_OUTPUT is for a different purpose -- i.e., to pass output between steps, not for any UI output)

I did not include the link that you had suggested because in order to see the message produced by these lines, you must actually be on the relevant build page, so the link would simply refer to the page on which this message appears (i.e., it would be redundant). In other words, the only place this message appears is on the very page given by the link. It would really only make sense to include the link in the message if the message appeared on some other page.

Thanks for getting me back on track! (1) Exactly, thank you :) I think for (2) I was thinking of text we could have a bot post into the PR as a comment but I didn't explain myself. Apologies! That might be a fun future improvement :)

Thanks for getting me back on track! (1) Exactly, thank you :) I think for (2) I was thinking of text we could have a bot post into the PR as a comment but I didn't explain myself. Apologies! That might be a fun future improvement :)

I totally agree with you on having a bot post a PR comment. That would be fantastic. I'll create a new issue for that improvement.

chuckwondo added 6 commits September 18, 2024 18:49

Fix/clarify grammar in development setup guide

8a14d90

Correct nox unit test session name

b1cdf3d

Fix workflow output name for python version

f72fa09

Name "checkout" step for clarity/consistency

fa4f5fb

Allow maintainer to re-run int. tests for fork PRs

007df73

Fixes nsidc#810

chuckwondo requested review from mfisher87, betolink and jhkennedy September 18, 2024 23:12

chuckwondo changed the title ~~Allow maintainer to run integration tests on PR from a fork~~ Allow maintainers to run integration tests on PR from a fork Sep 19, 2024

betolink approved these changes Sep 20, 2024

View reviewed changes

jhkennedy approved these changes Sep 20, 2024

View reviewed changes

chuckwondo merged commit 2939f80 into nsidc:main Sep 20, 2024
9 checks passed

mfisher87 reviewed Sep 21, 2024

View reviewed changes

This was referenced Sep 23, 2024

Update contributing doc to include instructions to set up Earthdata Login credentials on a fork so integration tests can run on PRs #773

Closed

Automatically add comment to PR that requires maintainer to review and rerun integration tests #824

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow maintainers to run integration tests on PR from a fork #818

Allow maintainers to run integration tests on PR from a fork #818

chuckwondo commented Sep 18, 2024 •

edited by github-actions bot

Loading

codecov-commenter commented Sep 18, 2024

chuckwondo commented Sep 18, 2024 •

edited

Loading

chuckwondo commented Sep 20, 2024

betolink commented Sep 20, 2024

betolink left a comment

betolink Sep 20, 2024

betolink Sep 20, 2024

jhkennedy Sep 20, 2024

chuckwondo Sep 20, 2024

jhkennedy left a comment

jhkennedy Sep 20, 2024

chuckwondo commented Sep 20, 2024

mfisher87 Sep 21, 2024 •

edited

Loading

chuckwondo Sep 23, 2024

mfisher87 Sep 25, 2024 •

edited

Loading

chuckwondo Sep 25, 2024

mfisher87 Sep 25, 2024

chuckwondo Sep 26, 2024

Allow maintainers to run integration tests on PR from a fork #818

Allow maintainers to run integration tests on PR from a fork #818

Conversation

chuckwondo commented Sep 18, 2024 • edited by github-actions bot Loading

codecov-commenter commented Sep 18, 2024

Codecov Report

chuckwondo commented Sep 18, 2024 • edited Loading

chuckwondo commented Sep 20, 2024

betolink commented Sep 20, 2024

betolink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhkennedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckwondo commented Sep 20, 2024

mfisher87 Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisher87 Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckwondo commented Sep 18, 2024 •

edited by github-actions bot

Loading

chuckwondo commented Sep 18, 2024 •

edited

Loading

mfisher87 Sep 21, 2024 •

edited

Loading

mfisher87 Sep 25, 2024 •

edited

Loading