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

Fix /build slash command to checkout the correct PR branch #2542

Merged
merged 58 commits into from
Apr 12, 2023

Conversation

echoix
Copy link
Collaborator

@echoix echoix commented Apr 8, 2023

The /build slash command was not checking out the correct branch when used. It was always using the base branch of the upstream repo. This was because it was called from a workflow_dispatch, so not tied to any event, nor pull request. The dispatcher (another workflow run), only had access to an issue event context, that doesn’t include information from the repo of the pull request. So, in order to hand the checkout action appropriate inputs, the build command is passed additional inputs, for the ref and repo name. These are obtained by using an api call from the pull request number in the repo (since it is available), and extracting the fields from the json.

I tried using some pre-made actions, but didn’t get the repo name easily, and they weren’t mainstream. So, it is just using a custom script with actions/github-script to do the api call, and set outputs.

Documentation for the usage is the same, it should do exactly the same as expected.

Fixes # … I’m not sure we reported it officially, but we (maintainers) are aware that it didn’t work directly as wanted.

Proposed Changes

  1. ...
  2. ...
  3. ...

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

echoix and others added 30 commits April 4, 2023 19:10
Add checkout-ref and checkout-repository
Add checkout-ref and checkout-repository inputs
Update slash-command-dispatch.yml 4
Update slash-command-dispatch.yml
Update slash-command-dispatch.yml
Update slash-command-dispatch.yml 8
Update slash-command-dispatch.yml9
@echoix echoix added github_actions Pull requests that update Github_actions code automation labels Apr 8, 2023
@echoix echoix self-assigned this Apr 8, 2023
@echoix echoix requested a review from nvuillam as a code owner April 8, 2023 18:06
@echoix
Copy link
Collaborator Author

echoix commented Apr 8, 2023

🦙 MegaLinter status: ⚠️ WARNING

Descriptor Linter Files Fixed Errors Elapsed time
✅ BASH bash-exec 6 0 0.01s
✅ BASH shellcheck 6 0 0.14s
✅ BASH shfmt 6 0 0 0.42s
✅ COPYPASTE jscpd yes no 2.83s
✅ DOCKERFILE hadolint 116 0 19.72s
✅ JSON eslint-plugin-jsonc 21 0 0 2.44s
✅ JSON jsonlint 19 0 0.22s
✅ JSON v8r 21 0 14.69s
⚠️ MARKDOWN markdownlint 312 0 230 7.83s
✅ MARKDOWN markdown-link-check 312 0 6.22s
✅ MARKDOWN markdown-table-formatter 312 0 0 24.1s
✅ OPENAPI spectral 1 0 1.51s
⚠️ PYTHON bandit 185 54 2.25s
✅ PYTHON black 185 0 0 5.17s
✅ PYTHON flake8 185 0 1.88s
✅ PYTHON isort 185 0 0 0.88s
✅ PYTHON mypy 185 0 8.07s
✅ PYTHON pylint 185 0 12.66s
⚠️ PYTHON pyright 185 251 17.96s
✅ PYTHON ruff 185 0 0 0.47s
✅ REPOSITORY checkov yes no 37.87s
✅ REPOSITORY git_diff yes no 0.4s
✅ REPOSITORY secretlint yes no 15.93s
✅ REPOSITORY trivy yes no 34.77s
✅ SPELL cspell 753 0 23.19s
✅ SPELL misspell 572 0 0 1.07s
✅ XML xmllint 3 0 0 0.42s
✅ YAML prettier 81 0 0 3.21s
✅ YAML v8r 23 0 61.35s
✅ YAML yamllint 82 0 1.3s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

@echoix
Copy link
Collaborator Author

echoix commented Apr 8, 2023

Limitations I forgot/couldn’t easily test yet:

  • Check that when used from another repo, with another unrelated user, the pulled branch (from the PR) can be pushed (has write access). Obviously the user making the PR should have checked the box to allow maintainers to update the branch. You see why I couldn’t test it easily by myself, with my PRs and my fork.
  • Check that the ref=… argument still works, or still works the same way as it is written in the output of the /help command, and the contribution guide. This I forgot to test

@echoix
Copy link
Collaborator Author

echoix commented Apr 8, 2023

Please squash-merge rather than merge this PR. I had to make a lot of commits (and merge what changing the dispatcher to test) since it was Actions-related work

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Apr 9, 2023

I think as long as the proper permissions block is specified, the first limitation will be/already is addressed. When someone forks a repo, those permissions being specified in the workflow keeps them the same between repositories even when some users have chosen to default them to read-all instead of write-all. I believe organization admins can limit those permissions, but there's nothing further we can do about that besides ask people to reopen the PR from a personal fork without such restrictions.

@echoix
Copy link
Collaborator Author

echoix commented Apr 9, 2023

That's not really what I meant actually. It's that when testing, the final state that I'm adding is like my 4th or 5th approach, and one of the failed attempts that was working was using a ref of refs/pull/${{ github.event.inputs.issue-number }}/head , but finally it is read-only, it couldn't be pushed to. So I just couldn't test that there isn't a remaining permission that would block in a same manner, that I couldn't have encountered yet.

@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Apr 9, 2023

Was ${{ github.event.inputs.issue-number }} the PR number? Do I understand correctly that you are asking whether pushing to <branch name> rather than refs/pull/<PR number>/head will work when future PRs are opened by non-maintainers who have checked the box to allow edits by maintainers, and a maintainer comments /build on their PR while logged into GitHub? If so, in principle, the contributor might have pretty complex branch protection rules that could result in a permission error that isn't preventable by speciying the proper workflow permissions. There's nothing we can do about that besides ask them to relax the branch protection rule(s) though, and it seems like a rare scenario. All refs of the form refs/pull/<PR number>/head are read-only on GitHub in all circumstances, so you did the right thing by using the branch name as the ref instead.

@echoix
Copy link
Collaborator Author

echoix commented Apr 9, 2023

Yes, that's what I'm not sure. And yes, the issue-number is the PR number. There is no distinction on that matter api-wise

@Kurt-von-Laven
Copy link
Collaborator

I edited my reply to basically say, in that case it sounds like you did the right thing.

@echoix
Copy link
Collaborator Author

echoix commented Apr 11, 2023

I think it is ready to merge, there's nothing left to do.

@nvuillam
Copy link
Member

@echoix except conflict it's ok :)

@bdovaz bdovaz enabled auto-merge (squash) April 12, 2023 19:21
@nvuillam nvuillam disabled auto-merge April 12, 2023 19:22
@nvuillam nvuillam merged commit cdec507 into oxsecurity:main Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation github_actions Pull requests that update Github_actions code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants