-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
BugFix: Automatic Lint Fixes Pull Request Workflow #4380
base: main
Are you sure you want to change the base?
Conversation
Thanks for your PR :) I would prefer APPLY_FIXES commit & pull_request to remain as they are working today, to not mess with existing installations What you could do it creating a new mode APPLY_FIXES commit-new-branch :) |
Hi @nvuillam thanks for the reply. I dont think this PR should affect the APPLY_FIXES commit option. The pull request option I could never get to work. Im unsure if I was maybe setting it up wrong. We chatted about it here #4317. Happy to have it set it up as a third option too. However I think both commit-new-branch and pull_request would end up doing the same thing? My understanding the options are:
Ive updated the PR title to be a bit clearer. |
Do you have some experience with these kind of workflows on github actions? There are some limitations that are there to help you avoid blindly creating security holes. There's also newer repos that are a bit stricter when new pull requests are created by bots, that are considered like being from a fork. (I can't remember where to find it). Then, pull requests from forks have limited permissions and access to secrets, the maximum is read-only, whatever is set. A pull request containing changes in workflows, in particular when coming from a fork, it doesn't behave the same, especially in regards to access to secrets. Then, usually the people's next intuitions when face to that problem and reading the docs, is to use the pull_request_target event trigger instead, as it can have more permissions, but it is very dangerous to use it that way, as it made for another use case. In that trigger, the goal is to act on what is already in the repo (like the default branch), and act on the PR without using their untrusted input. It is useful for PR labeling for example. But you can absolutely not checkout the PR in it. (Actions checkout will try to prevent you, but if you do so manually, and then build/run code from that untrusted input, lots of damage can be done). You didn't really precise what "didn't work" in your tests, but there are multiple ways that it would be just by design. Some limitations can be avoided by using a PAT (personal access token), which is similar to a password replacement, so actions can be done in your name, just as if you have made them, but you can select the scopes. The question then is, should you be doing it, and what is the attack surface you're opening. |
@echoix Thank you very much for the quick and detailed reply! It was a couple of weeks ago when I had the issue. I outlined parts of the issues I faced here #4317. I will try today on this testing repo. https://github.com/ScottGibb/ci_playground and see what the issues were and report back :) |
@echoix. Im trialing out megalinter in this repo: https://github.com/ScottGibb/ci_playground/blob/main/.github/workflows/megalinter.yml So I copied the yml file directly from the the MegaLinter repo ReadMe and set the following environment variables like so: env: # Comment env block if you don't want to apply fixes
# Apply linter fixes configuration
APPLY_FIXES: all # When active, APPLY_FIXES must also be defined as environment variable (in github/workflows/mega-linter.yml or other CI tool)
APPLY_FIXES_EVENT: pull_request # Decide which event triggers application of fixes in a commit or a PR (pull_request, push, all)
APPLY_FIXES_MODE: pull_request # If APPLY_FIXES is used, defines if the fixes are directly committed (commit) or posted in a PR (pull_request) I then created a new branch on my own repo and made some simple markdown errors. Expecting Megalinter to flag this and create a PR to my PR fix it. The flow did this: https://github.com/ScottGibb/ci_playground/actions/runs/12371229140/job/34526870592?pr=14 Checking the base repository state
/usr/bin/git symbolic-ref HEAD --short
fatal: ref HEAD is not a symbolic ref
/usr/bin/git rev-parse HEAD
29a412be6223a0a0670e3edc9348b1f13fecf8d8
Working base is commit '29a412be6223a0a0670e3edc9348b1f13fecf8d8'
Error: When the repository is checked out on a commit instead of a branch, the 'base' input must be supplied. I initially thought this was a git error. Do you think this might be a token issue? Also thank you again for your detailed response about attack surfaces and PAT token. Im fairly new to PAT tokens and GHA especially with ones that change the code on your repo. Really interesting stuff! |
Fixes #4317
Proposed Changes
I'm proposing we change the PR Workflow for committing to the changes to a branch. I found when I used the existing workflow I could not get it to work properly. Thus I tweaked the workflow and got it working in this repo https://github.com/dysonltd/tmag5273/pulls?q=is%3Apr+is%3Aclosed as well as in my own personal CI testing repo ScottGibb/ci_playground#9.
Ive changed the GitHub Actions workflow so that it follows this process:
An example of this can be found cleanly on this ScottGibb/ci_playground#9
When speaking to @nvuillam he mentioned the following:
Hopefully this is suitable. If there are any changes let me know :)
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breaking
if this is a large fundamental changeautomation
,bug
,documentation
,enhancement
,infrastructure
, orperformance