markbind
markbind copied to clipboard
MarkBind
Utilize GitHub Action to aid PR review
Please confirm that you have searched existing issues in the repo
Yes, I have searched the existing issues
Any related issues?
No response
What is the area that this feature belongs to?
Other
Is your feature request related to a problem? Please describe.
Use GitHub Action, possibly available ones, to automate and remind PR authors of the potential flaws:
- [x] missing proposed commit message
- [x] broken link
- [ ] the need to update test/docs for certain changes (could be harder to achieve)
- For example, if we know that a piece of code is coupled to some documentation, can we detect that both the code and the documentation are updated?
- etc
Any other helpful points pls feel free to edit or comment below.
Describe the solution you'd like
As above
Describe alternatives you've considered
No response
Additional context
No response
utilizing our existing intra-site broken link detection could be a good first step:
- [x] a step in the CI that serve/builds the docs, check that the console does not contain broken link warning(or any other warning/error) logs
To add some background info: I was primarily looking to use danger.js for this issue. However, even though the tool itself is reasonably easy to integrate, it is not straightforward to use with a forking workflow because of permission issues (i.e, when someone is making a PR from a fork, the triggered GitHub Action has limited permission to prevent leaking secrets etc).
- relevant link if danger.js is to be used
- a presentation I made attempting to integrate dangerjs in markbind, in case it's helpeful
Another thing that I would be interested to do is to check if the PR is tagged with the release tags upon merging! I think this is something we always forget to do (opps) but helps make releases a lot easier.
I was thinking perhaps it can ping the person who merged if they forget to tag it.
Another thing that I would be interested to do is to check if the PR is tagged with the release tags upon merging! I think this is something we always forget to do (opps) but helps make releases a lot easier.
I was thinking perhaps it can ping the person who merged if they forget to tag it.
@KevinEyo1 will be taking up this issue
Hi, I'm working on the checking of the coupled code portion now. I have thought about how to implement this, some considerations up for discussion. (This also applies to if Danger.js is used, since it also requires write access and also requires checking out of code)
Current implementation idea (or use Danger.js) A file mapping list of files and their coupled test/docs file, which I think will have to be manually updated to provide better accuracy A script to handle the logic (can also be part of workflow) An action to handle checking of changes based on the mapping and script
This means that checking out of code is required, which means that pull_request_target should not be used for security reasons. A possible solution of setting the repo checked out to be the base branch repo, as per my research, does not work since the changes that we want to check from the PR would not be reflected in the base branch repo.
This means that there will be no write access given to the GITHUB_TOKEN since it is a PR from a forked repo, and hence I cannot ping the user with a comment.
However, I do not think directly failing the check is ideal either, at least for now since there could be hidden inaccuracies in the coupling of files, and would be problematic if it fails the test for a perfectly fine PR, preventing it from merging. But if it doesnt fail and just echoes something, then users might easily miss it.
Solutions:
One way to solve this is by still using pull_request_target anyways but add an additional step of running workflow only on approval by trusted users, such that the trusted user has to check the changes in the code from the PR to ensure there is no malicious code before running the workflow.
TLDR (all use Danger.js or a script)
Solution 1 (current implementation):
Use pull_request trigger event and echo an error and does not fail the action. (Less visibility)
Solution 2:
Use pull_request_target trigger event but type on approved for security reasons, which will then run and post a comment and does not fail the action. (More visibility but needs approval everytime)
Solution 3 (not ideal right now, but eventually ideal once accuracy is achieved):
Use pull_request trigger event and fail the action. (More visibility but requires coupling to be accurate)
Thank you @KevinEyo1 for the investigation and summary above!
However, I do not think directly failing the check is ideal either, at least for now since there could be hidden inaccuracies in the coupling of files, and would be problematic if it fails the test for a perfectly fine PR, preventing it from merging. But if it doesnt fail and just echoes something, then users might easily miss it.
The solution I would propose here is to make that CI check fail, but the branch protection to allow merging regardless of this check's status.