Allow maintainer to re-run integration tests for PRs from forks #811

chuckwondo · 2024-09-17T13:32:25Z

This is an attempt to fix #810. It may require merging before it can be fully confirmed to fix the problem because it may require the integration-test.yml file to be on the main branch before it will be properly triggered.

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.

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

chuckwondo · 2024-09-17T14:12:27Z

@mfisher87, this is an attempt to fix the failing integration tests issue (#810). My work on #808 exacerbates the issue, and after further research, I opened #810 and created this PR in an attempt to fix the problem. Unfortunately, I don't think we'll know for sure until this PR is actually merged to main, it's merged to #808, and that PR is then re-run by a maintainer to see if that then allows the int. tests to succeed.

If this works, the general workflow for a PR from a fork would be as follows:

PR from a fork is opened or updated (synchronized)
The int. tests fail (actually, they should be skipped with a failing status, giving a message that the user who submitted the PR does not have permissions to run int. tests
A maintainer performs a security review -- for known/trusted contributors, the security review should be almost a no-op
The maintainer re-runs the failed build, if there are no security concerns, which should then allow int. tests to run

Currently, the only way for int. tests to run is for a maintainer (someone with write permission to this repo) to submit an internal PR (i.e., from a branch within this repo, not from a fork) OR for a maintainer to merge a PR with failing int. tests. In the latter case, we must make an assumption that the int. tests will succeed once merged to main. If they don't then we might have a bit of a problem -- we may need to revert the merge, which is generally no fun.

With this PR, we should be able to get int. tests to run from forks prior to merging, so that if int. tests fail, we do not merge. The downside to this is that it requires a bit of manual effort from a maintainer to make sure the PR does not pose a security risk (which should not be the case for trusted contributors) since the changes in this PR make it such that the fork PR can read the secrets that such PRs cannot currently read (thus, the reason int. tests from a fork always fail), but only once a maintainer gives the thumbs up and re-runs the failed build (when this happens, the build is run with the maintainer's permission, which allows secrets to be read).

mfisher87 · 2024-09-17T15:58:05Z

Amazing. I was trying to do the same in the icepyx repository (https://github.com/mfisher87/icepyx/blob/test-with-gha/.github/workflows/integration_test.yml) and ended up giving up on it because I had higher priorities and limited time. What do you think of merging this to main on a fork, and then I could help you test by opening a PR against your fork from a fork-of-the-fork?

mfisher87 · 2024-09-17T16:01:54Z

.github/workflows/integration-test.yml

+      - name: Checkout source
+        if: ${{ github.event_name != 'pull_request_target' }}
+        uses: actions/checkout@v4
+      - name: Fetch user permission
+        if: ${{ github.event_name == 'pull_request_target' }}
+        id: permission
+        uses: actions-cool/check-user-permission@v2
+        with:
+          require: write
+          username: ${{ github.triggering_actor }}
+      - name: Check user permission
+        if: ${{ github.event_name == 'pull_request_target' && steps.permission.outputs.require-result == 'false' }}
+        # If the triggering actor does not have write permission (i.e., this is a
+        # PR from a fork), then we exit, otherwise most of the integration tests will
+        # fail because they require access to secrets.  In this case, a maintainer
+        # will need to make sure the PR looks safe, and if so, manually re-run the
+        # failed jobs.
+        run: |
+          echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."
+          echo "A maintainer must perform a security review and re-run this build, if the code is safe."
+          exit 1
+      - name: Checkout source
+        if: ${{ github.event_name == 'pull_request_target' }}
+        uses: actions/checkout@v4
+        with:
+          ref: ${{ github.event.pull_request.head.sha }}


Suggested change

- name: Checkout source

if: ${{ github.event_name != 'pull_request_target' }}

uses: actions/checkout@v4

- name: Fetch user permission

if: ${{ github.event_name == 'pull_request_target' }}

id: permission

uses: actions-cool/check-user-permission@v2

with:

require: write

username: ${{ github.triggering_actor }}

- name: Check user permission

if: ${{ github.event_name == 'pull_request_target' && steps.permission.outputs.require-result == 'false' }}

# If the triggering actor does not have write permission (i.e., this is a

# PR from a fork), then we exit, otherwise most of the integration tests will

# fail because they require access to secrets. In this case, a maintainer

# will need to make sure the PR looks safe, and if so, manually re-run the

# failed jobs.

run: |

echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."

echo "A maintainer must perform a security review and re-run this build, if the code is safe."

exit 1

- name: Checkout source

if: ${{ github.event_name == 'pull_request_target' }}

uses: actions/checkout@v4

with:

ref: ${{ github.event.pull_request.head.sha }}

- name: Fetch user permission

if: ${{ github.event_name == 'pull_request_target' }}

id: permission

uses: actions-cool/check-user-permission@v2

with:

require: write

username: ${{ github.triggering_actor }}

- name: Check user permission

if: ${{ github.event_name == 'pull_request_target' && steps.permission.outputs.require-result == 'false' }}

# If the triggering actor does not have write permission (i.e., this is a

# PR from a fork), then we exit, otherwise most of the integration tests will

# fail because they require access to secrets. In this case, a maintainer

# will need to make sure the PR looks safe, and if so, manually re-run the

# failed jobs.

run: |

echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."

echo "A maintainer must perform a security review and re-run this build, if the code is safe."

exit 1

- name: Checkout source

uses: actions/checkout@v4

with:

ref: ${{ github.event.pull_request.head.sha }}

I think we can simplify to one checkout -- if the event_name is pull_request_target, we'll fail before we reach that point due to the permissions checks.

EDIT: What a repulsive diff output. I only removed the first checkout, and removed the conditional from the second checkout.

Yes, I think we can simplify this, but we don't want to only one checkout because a "push" event will not have a pull request sha. I'll rework.

Since the default for ref is "" (IIRC), can we do ${{ github.event.pull_request.head.sha | '' }}?

mfisher87 · 2024-09-17T16:04:22Z

.github/workflows/integration-test.yml

      - name: Set up Python
        uses: actions/setup-python@v5
        with:
          python-version: ${{ matrix.python-version }}
      - name: Get full python version
        id: full-python-version
-        run: echo ::set-output name=version::$(python -c "import sys; print('-'.join(str(v) for v in sys.version_info))")
+        run: echo version=$(python -c "import sys; print('-'.join(str(v) for v in sys.version_info))") >> $GITHUB_OUTPUT


This is fixed and refactored in #733

Ah, right. I'll remove from this PR so we don't get a merge conflict.

mfisher87 · 2024-09-17T16:05:32Z

.github/workflows/integration-test.yml

+          echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."
+          echo "A maintainer must perform a security review and re-run this build, if the code is safe."


Suggested change

echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."

echo "A maintainer must perform a security review and re-run this build, if the code is safe."

echo "User ${{ github.triggering_actor }} does not have permission to run integration tests." | tee -a $GITHUB_OUTPUT

echo "A maintainer must perform a security review and re-run this build, if the code is safe (see https://securitylab.github.com/resources/github-actions-preventing-pwn-requests)." | tee -a $GITHUB_OUTPUT

echo "Re-run at ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" | tee -a $GITHUB_OUTPUT

It'd be awesome to expose this at the GUI. Untested URL :)

chuckwondo added 2 commits September 17, 2024 09:28

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

2ccf1a6

Add back pull_request action

fa49692

chuckwondo marked this pull request as ready for review September 17, 2024 14:12

chuckwondo requested a review from mfisher87 September 17, 2024 14:12

chuckwondo changed the title ~~Allow maintainer to re-run integration tests for fork PRs~~ Allow maintainer to re-run integration tests for PRs from forks Sep 17, 2024

mfisher87 previously approved these changes Sep 17, 2024

View reviewed changes

mfisher87 mentioned this pull request Sep 17, 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

Simplify checkout

bd4d9f9

chuckwondo dismissed mfisher87’s stale review via bd4d9f9 September 17, 2024 17:29

chuckwondo closed this Sep 18, 2024

chuckwondo deleted the integration-tests-issue-810 branch September 18, 2024 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow maintainer to re-run integration tests for PRs from forks #811

Allow maintainer to re-run integration tests for PRs from forks #811

chuckwondo commented Sep 17, 2024 •

edited by github-actions bot

Loading

chuckwondo commented Sep 17, 2024

mfisher87 commented Sep 17, 2024

mfisher87 Sep 17, 2024

chuckwondo Sep 17, 2024

mfisher87 Sep 17, 2024

mfisher87 Sep 17, 2024

chuckwondo Sep 17, 2024

mfisher87 Sep 17, 2024 •

edited

Loading

		echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."
		echo "A maintainer must perform a security review and re-run this build, if the code is safe."

-          echo "User ${{ github.triggering_actor }} does not have permission to run integration tests."
-          echo "A maintainer must perform a security review and re-run this build, if the code is safe."
+          echo "User ${{ github.triggering_actor }} does not have permission to run integration tests." | tee -a $GITHUB_OUTPUT
+          echo "A maintainer must perform a security review and re-run this build, if the code is safe (see https://securitylab.github.com/resources/github-actions-preventing-pwn-requests)." | tee -a $GITHUB_OUTPUT
+          echo "Re-run at ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}" | tee -a $GITHUB_OUTPUT

Allow maintainer to re-run integration tests for PRs from forks #811

Allow maintainer to re-run integration tests for PRs from forks #811

Conversation

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

chuckwondo commented Sep 17, 2024

mfisher87 commented Sep 17, 2024

mfisher87 Sep 17, 2024

Choose a reason for hiding this comment

chuckwondo Sep 17, 2024

Choose a reason for hiding this comment

mfisher87 Sep 17, 2024

Choose a reason for hiding this comment

mfisher87 Sep 17, 2024

Choose a reason for hiding this comment

chuckwondo Sep 17, 2024

Choose a reason for hiding this comment

mfisher87 Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

chuckwondo commented Sep 17, 2024 •

edited by github-actions bot

Loading

mfisher87 Sep 17, 2024 •

edited

Loading