Add split workflow, allowing the action to be used in PRs created from forks

[pajlada]

Why python:3 docker base image?

For the post_comments workflow, I've decided to use the python:3 docker image from dockerhub.
This image is based off of debian.
Because no system binaries are run other than python, I don't expect any issues with the Ubuntu <-> Debian jump. Being able to skip any apt calls speeds up the workflow by a good chunk.

What's changed

A lot of the code from review.py has been moved to a package clang_tidy_review stored in the path /post_comments/clang_tidy_review - this is to ensure the package can be used by both the review and post_comments workflows.

The main workflow Dockerfile has been updated to include the post_comments directory, this is done to include the shared library contained within that directory

The main workflow has a new input: post_comments.
This input controls whether comments should be posted by this workflow or not.
Action item: Does this input name accurately describe its functionality?

• Make the split workflow work with forks
• Post an error annotation if the regular workflow is used with a link to the "How to use the split workflow" page
• Add a section to the main README.md page to describe how and why to use the split workflow

Fixes #49

Example PR showing the error annotation: pajlads/test-cpp-project#2 - it's not showing up straight in the PR page because it's not linked to a file, rather you need to click details on the failed action. If this is something we very much want we could potentially use the first file from the review that was about to be posted to have it show up more like a PR comment

This PR is hefty, and contains a lot of different things done in the same PR. I'd suggest this is either squash & merged, or let me know and I'll clean up the commits to more manageable pieces before it gets merged in.

Looking forward to hear your feedback!

To test this right now, you can point your action at my fork like done in my test project: https://github.com/pajlada/mini-cpp-project/blob/master/.github/workflows/build.yml and https://github.com/pajlada/mini-cpp-project/blob/master/.github/workflows/post_comments.yml

The downloading and uploading of artifacts could be done by just using the raw github api using the requests library, which would make it a lot cleaner to use the split workflow.

[pajlada]

@FlorianReimold This is now in a workable state, I've confirmed this works in a way that would work for me in my projects that get a lot of pull requests from forks.

How it's actually implemented right now with a mix of other github-script actions and what-not is not really how I envision it looking when this PR gets its final polish, but it's ready to be played with at least!

[FlorianReimold]

@pajlada: Thanks for this gigantic PR! I am going to test out your clang-tidy-review version as soon as possible and report back in the next days!

[FlorianReimold]

I gave it a rough test. It worked. I would totally use it in the exact state it is in.

What I still have noticed:

• There is no LGTM comment anymore. This is a little inconsistent, but honestly, I don’t miss it at all. It never provided a real value, only spammed everybody with notifications and emails.
• The number of comments isn’t set any more, so you cannot “fail” the main clang-tidy-review workflow anymore if clang-tidy has detected issues
• The Post-comments script is always taken from master. I worked on two temporary branches first and the clang-tidy-review action would work, but the post-comments action would not trigger at all. It started working when I pushed the actions to master.
• The Post clang-tidy review action fails, whenever clang-tidy-review didn’t detect any issue, as the workflow cannot download the file. This clutters the overall appearance of the Actions tab, as every success (no clang-tidy findings) is accompanied by a failed post-comments workflow.
• The readme.md should be adapted. Currently it contains an example for [email protected] 😉

[ZedThree]

Thanks @pajlada for this, and also @FlorianReimold for testing it!

Florian's second point is quite important -- some people like to use the action as pass/fail rather than just comments, so it's important to get that working.

My main suggestion is to undo the move of post_comments out of review, and instead have the main action always create the comments as a JSON file and upload that as an artefact. Then, the second post_comments action could be a javascript action instead of docker. That would have a couple of advantages:

1. it would be much faster as it wouldn't need to build or even download the docker container
2. it could incorporate downloading the comments json artefact so the user would only need a single step in their workflow yaml file.

Admittedly it would involve writing some javascript, but it should just be able to use the github octokit library to upload the comments straight from the json file

[pajlada]

Thank for your feedback folks!

I will look to make the requested changes this weekend and add some overall polish

Non-exhaustive list for myself:

• Update/review README.md examples - This should be done in a release PR/commit when this has been merged in, current examples explain the fork process well enough for now imo
• Fix LGTM comment
• Ensure output total_comments is correctly output from both actions, regardless of post_comments input value
• Explore javascript/octokit post_comments action

The Post-comments script is always taken from master. I worked on two temporary branches first and the clang-tidy-review action would work, but the post-comments action would not trigger at all. It started working when I pushed the actions to master.

This is actually on purpose, the way to escalate and get permissions is through workflow_run which runs everything from the main branch of the repo, that way the user can't add new code to a malicious workflow that leaks secrets without you merging it into the main branch.

[FlorianReimold]

Just to give you my opinion on the javascript post-comments workflow: Honestly, I wouldn't care too much about that. The current python3-docker based version completes in about 60 seconds, which is totally reasonable. (Also I don't know javascript 😉)

Example run: https://github.com/FlorianReimold/ecal-test-snapshot/actions/runs/2868416302

[pajlada]

To begin, sorry for the big PR! After 49 force pushes I believe I've done all I can to make it as easy to review as possible. It's now in a state that I'm happy with, so I believe there's just a few things left.

Move all shared logic from review.py into the clang_tidy_review library This commit only moves code and is the biggest contributor to the final lines added/removed.

What I have not done, is convert the post_comments child action to javascript yet. This can still be done, but it's now a bit easier to see that the javascript action would need to duplicate quite a lot of logic, namely:

• Searching for previous comments
• Limiting the amount of comments to make
• The LGTM comment posting logic (find last comment, skip posting LGTM if last comment was LGTM (which is broken now btw but not in scope of this PR to fix))

The workflow necessary to download/upload the artifact is still ugly right now, but this can be improved in python by making REST API requests ourselves (not relying on PyGithub)

Questions:

1. Do you still want the post_comments child action converted to javascript?
2. If no to 1., do you want the workflow cleanup as part of this pull request or as another pull request?
3. Naming! I've come to dislike the name of the action and option post_comments. I think the name should instead be post_review as that's what it's doing. Thoughts?

[FlorianReimold]

3. Naming! I've come to dislike the name of the action and option post_comments. I think the name should instead be post_review as that's what it's doing. Thoughts?

First of all I would also prefer post_reviews, as that is more in line with what it actually does. When I applied the previous dev version I however found it a little counterintuitive to set "post_comments=false" in order to get the two-step-workflow working - I mean I actually want it to post comments, but from another workflow. I don't really have a better name for it though. Maybe "use_split_workflow=true" or something like that...

[pajlada]

3. Naming! I've come to dislike the name of the action and option post_comments. I think the name should instead be post_review as that's what it's doing. Thoughts?

First of all I would also prefer post_reviews, as that is more in line with what it actually does. When I applied the previous dev version I however found it a little counterintuitive to set "post_comments=false" in order to get the two-step-workflow working - I mean I actually want it to post comments, but from another workflow. I don't really have a better name for it though. Maybe "use_split_workflow=true" or something like that...

I like that naming idea a lot, makes it more of a conscious choice in my head that you are going to be using the action in an entirely different way

[ZedThree]

+1 for split_workflow 🙂

Leaving the workflow as Python for now seems like the sensible way forward, and maybe I'll look at converting it to javascript in future

[FlorianReimold]

Hey everybody, we don't want this PR to die, right? 😊
I am currently using an old fork in our main repository and it works perfectly (OK, well, there is an issue, but it has nothing to do with the split workflow). The fork is at the state when I tried it out last time.

Are we waiting for me to approve the current workflow again? I can try it out and see if I find issues or room for improvements! 👍

[pajlada]

Hey everybody, we don't want this PR to die, right? blush I am currently using an old fork in our main repository and it works perfectly (OK, well, there is an issue, but it has nothing to do with the split workflow). The fork is at the state when I tried it out last time.

Are we waiting for me to approve the current workflow again? I can try it out and see if I find issues or room for improvements! +1

I've been a bit busy on other work recently, I think this PR is in a pretty good state and I've been testing it with pull requests in this repo

It runs into some undiscoverable issues sometimes where the split workflow fails because the artifact uploading failed - I'm not sure how this could be made more discoverable, but it could be handled better.

The workflows I'm running now:

1. Build https://github.com/Chatterino/chatterino2/blob/master/.github/workflows/build.yml#L149-L164
2. Post review https://github.com/Chatterino/chatterino2/blob/master/.github/workflows/post-clang-tidy-review.yml

[ZedThree]

I'm very busy this week, but I'll try and have a proper look when I can

[FlorianReimold]

It runs into some undiscoverable issues sometimes where the split workflow fails because the artifact uploading failed - I'm not sure how this could be made more discoverable, but it could be handled better.

Hm maybe you could just always create a file, even if it is empty? The download should then more or less never fail, just because there should always be a file present 🤔

[ZedThree]

Thanks @pajlada and @FlorianReimold for writing and testing this! I haven't yet found a good way of testing this Action except manually, so I really appreciate the effort you've both put in. I'm going to merge this now so that other people can start using it.

[ZedThree]

You can use this now in v0.10.0