-
-
Notifications
You must be signed in to change notification settings - Fork 607
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
Add GH action format check files with JuliaFormatter
#2074
Conversation
Perhaps the title would be better with On formatting, we should use the same style across all FluxML repos. Just copying https://github.com/FluxML/Metalhead.jl/blob/master/.JuliaFormatter.toml should be enough, I think. |
Are you referring to the PR's title or the commit message of the bot's commit? (Should it be -
Thanks! I will add this in! |
This works now! Working on a PR created from the base repository - Saransh-cpp#53 Note
Why is PAT required?A bot commit without a PAT does not trigger workflows. This means that the reformatted files will not go through the conventional CI, which can potentially be dangerous. Let's say a bug in Is this a security concern?Since this workflow runs on pull_request_target, the PAT itself cannot be leaked. I have successfully tested this here - Saransh-cpp#55 (see the files changed in the last commit - I tried printing out the PAT that, in a sense, is an attempt to hack FluxML's private tokens) -> https://github.com/Saransh-cpp/Flux.jl/actions/runs/3175153155/jobs/5172838261 (see how the "HACK" step was not executed at all). This shows that GitHub will run the "Lint" workflow according to the instructions written in the base Due to this reason, we cannot test this action on this PR (as
|
.github/workflows/lint.yml
Outdated
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- uses: actions/checkout@v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise.
https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you already quoted this portion above:
But the repository owners must be extra careful not to trigger any script that may operate on PR controlled contents like in the case of npm install.
This is the problem. Combining pull_request_target
with a checkout is a huge footgun. Can it be done safely? Potentially, yes. But it is so easy to make a mistake here. All it takes is for a committer to merge a PR with an innocent looking change (e.g. changing julia
to julia --project
in the lint workflow), and the repository has been compromised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My recommendation would be to not go with this approach, because it is too easy to end up compromising the repository.
Instead, if the goal is to enforce a common format across all repositories in the FluxML organization, I would recommend:
- Have a CI job that checks if the source code is formatted correctly, and errors if any formatting problems are detected.
- Use the
needs
construct to force all other CI jobs to wait for the successful completion of the formatting check. - If the formatting check does not succeed, do not run any other CI jobs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, in my above comment, change "potentially compromise" to "compromise". This PR already compromises the repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @DilumAluthge! Good thing I cross-posted this in ci-dev
. I agree, this workflow is potentially risky. If someone merges a PR that changes the workflows then Flux would be at a security risk. Leaving out the PAT completely is equally risky as that would allow the GitHub actions bot to commit anything without the CI running on it.
This PR already compromises the repository.
Could you let me know? Is it that one can write a script to leak the PAT in another workflow? I tried this in one of my PRs, but GitHub does not allow the user to access the PAT -
- name: POTENTIAL HACK
run: echo ${{ secrets.PAT }}
After another read-through, I agree with @DilumAluthge's concerns and raise one of my own. If the bot is appending formatting commits to PR branches, how are contributors meant to continue working on their feature branches from another checkout without running into conflicts? #2074 (comment) seems like a more reasonable approach and is already used in projects like https://github.com/JuliaDiff/ChainRulesCore.jl/blob/main/.github/workflows/format.yml. To ensure code is formatted in the remote branch, we could also consider a pre-commit hook.
Both. Your proposed rename looks fine. |
A |
But printing the |
JuliaFormatter
FWIW the formatter on ChainRulesCore.jl seems more irritating than helpful. It clutters actual review with lots of comments telling you that An ideal auto-formatter that results in less time spent thinking about spacing might be nice. But we spend very little. And a half-baked one which creates more fiddling seems much worse than where we are now. Other concerns here are:
|
IMO the most important part of an autoformatter is that it enforces
GitHub supports using |
Yes! Right now this uses the config specified in |
TODO
|
JuliaFormatter
JuliaFormatter
Looks better now. Should the formatting options be discussed in the next community call? Here is the current configuration - style = "sciml"
whitespace_in_kwargs = true
format_docstrings = true
always_for_in = true
join_lines_based_on_source = true
separate_kwargs_with_semicolon = true
always_use_return = true
margin = 92
indent = 4 |
|
Given that the PR to add Pre-commit support will make setting up a formatting workflow extremely easy and reliable. Please let me know if this PR is still required. Closing this for now :( |
I think it would be better if we didn't have to rely on pre-commit (which is a heavy dependency and requires a Python interpreter). A solution which works with pure Julia would be better and should be doable, someone just has to write it. The other blocker is the JuliaFormatter latency from the command line is really only acceptable on 1.9+. If the binary distribution story can be figured out before 1.9 becomes ubiquitous, then we can consider that route instead. |
I hadn't factored in adding pre-commit as a dependency. Trying out JuliaFormatter after 1.9 is out of Beta sounds great. I think it would be better to create a new PR when 1.9 is out. Resolving conflicts in this PR is getting more and more difficult with every update in Flux's codebase 😅 |
Most of the Julia packages I have seen lack a proper style guide for their codebase. I feel
JuliaFormatter
coupled with a clever workflow can help us maintain a clean codebase. This PR adds a bot-like workflow that fixes the formatting issues associated with a PR and pushes the fixed files in the same branch.PR Checklist