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.

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

Support Workflow Runs #281

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

techman83
Copy link

Due to security issues running untrusted checkouts from forks, access to things like secrets and tokens are blocked. It is possible to allow this, but it is not wise, and the recommended process is to use a workflow run call. This will add support for workflow_run based pull requests, allowing PRs to be commented upon, regardless of whether they are a fork or from a local branch.

@techman83
Copy link
Author

I feel like the test for this may suffer from the same problem as this one solves 🙂

HttpError: Resource not accessible by integration
    at /home/runner/work/coverage/coverage/webpack:/typescript-action/node_modules/@octokit/request/dist-node/index.js:86:1
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

@techman83
Copy link
Author

I've split the workflows in the same manner as my PR recommends in terms of usage. It is notable that for this PR, coverage won't run, as the workflow needs to be in the default branch before it will become an action that can be triggered.

@techman83
Copy link
Author

@orgoro anything extra required for merge? I've been using my fork in my own workflows for a while now 🙂

@tdstein
Copy link

tdstein commented Jun 3, 2024

@techman83 - thanks for putting this together. I tried it out but ran into an error: https://github.com/posit-dev/posit-sdk-py/actions/runs/9353058789

It looks like the 'base' variable is undefined, which I believe stems from here: https://github.com/orgoro/coverage/pull/281/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R19

I tried to determine the shape of context.payload.workflow_run.pull_requests[0] but couldn't figure it out.

Any thoughts or advice is appreciated. Thanks!

@techman83
Copy link
Author

@tdstein I'm not really sure, it has been working pretty reliably for most of my testing

The payload is what is in the github context, so being a workflow run, in here should be where it's at.
https://github.com/orgoro/coverage/pull/281/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R23

Would need a sample of the github to get a better idea. Which you can dump using echo

    - run: echo "${{ toJson(github) }}"

It would be nice if actions/core had a workflow type that could be consumed, but that extended well beyond my available time for such things 🙂

Due to security issues running untrusted checkouts from forks, access to things
like secrets and tokens are blocked. It is possible to allow this, but it is
not wise, and the recommended process is to use a workflow run call. This will
add support for workflow_run based pull requests, allowing PRs to be commented
upon, regardless of whether they are a fork or from a local branch.

fixes orgoro#259
Cover workflow fails for the same reason as this PR is attempting to
resolve. Splitting them in the same manner.
@techman83
Copy link
Author

Rebased off current main 🙂

@cardoe
Copy link

cardoe commented Nov 13, 2024

@orgoro can we land this?

@techman83
Copy link
Author

techman83 commented Nov 14, 2024

This is actually going to need re-work. GitHub do not include the 'pull_request' payload from forks, resulting in the triggered workflow failing as the key is not in the object.

Error: "Cannot read properties of undefined (reading 'base')"

What needs to happen is that along with the coverage.xml, a coverage.pr file needs to be in the upload artifact, ie

      - name: Upload Coverage
        uses: actions/upload-artifact@v4
        with:
          name: coverage
          path: coverage.*
          retention-days: 1

With contents of the pull request number:

123

Followed by adding logic in the code to pull that from the API. If someone can stub out the Typescript for this it'd give me a good head start, otherwise I'll need to find some time (it's been a little in short supply lately!).

The big bonus is that it will decouple the reporting from the event. So it will become usable by any triggering workflow, so long as that PR file is included with the relevant PR to comment on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants