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

Utilize GitHub Action to aid PR review #2140

Open
2 of 3 tasks
tlylt opened this issue Feb 7, 2023 · 7 comments
Open
2 of 3 tasks

Utilize GitHub Action to aid PR review #2140

tlylt opened this issue Feb 7, 2023 · 7 comments
Assignees
Labels
a-DevOps c.Feature 🚀 p.Medium s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding

Comments

@tlylt
Copy link
Contributor

tlylt commented Feb 7, 2023

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:

  • missing proposed commit message
  • 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

@tlylt
Copy link
Contributor Author

tlylt commented Apr 9, 2023

utilizing our existing intra-site broken link detection could be a good first step:

  • 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

@tlylt tlylt added the s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding label Apr 9, 2023
@tlylt tlylt added p.Medium a-DevOps d.easy and removed s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding labels Apr 16, 2023
@KevinEyo1 KevinEyo1 self-assigned this Feb 5, 2024
@tlylt
Copy link
Contributor Author

tlylt commented Feb 7, 2024

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

@kaixin-hc kaixin-hc added s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding and removed d.easy labels Feb 25, 2024
@yucheng11122017
Copy link
Contributor

yucheng11122017 commented Mar 13, 2024

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.

@yucheng11122017
Copy link
Contributor

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

@KevinEyo1
Copy link
Contributor

KevinEyo1 commented Apr 8, 2024

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.

@KevinEyo1
Copy link
Contributor

KevinEyo1 commented Apr 15, 2024

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)

@tlylt
Copy link
Contributor Author

tlylt commented Jun 23, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-DevOps c.Feature 🚀 p.Medium s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding
Projects
None yet
Development

No branches or pull requests

4 participants