ci: Add PR review bot

[jcs090218]

For #6714.

A few results:

• Test 8900 jcs-PR/melpa#23 (Test Create sampie-theme #8900)
• Test 8892 jcs-PR/melpa#24 (Test [recipe]: Add app-monochrome #8892)

The workflow file in steps:

1. Check to see if the PR is good to review (job check)
2. Install Emacs and melpazoid to test the package based on the PR number
3. Comment the result back to PR

This is one simple approach. Please let me know what you think!

[port19x]

Couldn't we just omit the check section?
Why add a comment instead of just CI status via exit 0/1?

• Perhaps this can be extended to use emacs 28 and 30 too and do so in parallel.

[port19x]

rationale: ignore draft PRs

Suggested change

	pull_request:
	paths:
	- 'recipes/**'
	pull_request:
	types: [opened, synchronize, reopened, ready_for_review]
	paths:
	- 'recipes/**'

[port19x]

rationale: parallel multi-version build

Suggested change

	review:
	review:
	strategy:
	matrix:
	version: [28.2, 29.2, 30]

[port19x]

rationale: parallel multi-version build

Suggested change

	version: 29.2
	version: ${{ matrix.version }}

[port19x]

rationale: ignore draft PRs

Suggested change

	if: ${{ needs.check.outputs.status == 'success' }}
	if: ${{ needs.check.outputs.status == 'success' }}
	if: github.event.pull_request.draft == false

[jcs090218]

Couldn't we just omit the check section?

I don't prefer doing that since it will result a red mark ❌. Another approach is to use continue-on-error, but that's too messy compare to my approach.

Why add a comment instead of just CI status via exit 0/1?

Those checks are opinionated, so it's better to post the comment out. Furthermore, I reckon people are unlikely to review the GitHub Actions log page to identify the error. 🤔

[port19x]

I don't prefer doing that since it will result a red mark ❌

That's kind of the point of CI, isn't it?
If melpazoid can't run due to a missing repo link, despite the PR template, that should be marked red.
Ideally we can rename check to sth like check-repo-link, that way it is even more apparent to the PR author where the red x comes from.
If melpazoid can run, perhaps it pulls the source from the recipe itself, then check is unnecessary.

I reckon people are unlikely to review the GitHub Actions log page

Fair point, also since the melpa check does basically a 4 in 1, you can't see by what exactly failed where to work.
If there were separate github actions for byte-compiling, package-lint, checkdoc, etc. this would be less of an issue.

[jcs090218]

That's kind of the point of CI, isn't it?
If melpazoid can't run due to a missing repo link, despite the PR template, that should be marked red.
If melpazoid can't run due to a missing repo link, despite the PR template, that should be marked red.
If melpazoid can run, perhaps it pulls the source from the recipe itself, then check is unnecessary.

PR doesn't always refer to a new package. Like this PR is not relevant to "Adding a new recipe", making it red will only cause confusions.

Fair point, also since the melpa check does basically a 4 in 1, you can't see by what exactly failed where to work.
If there were separate github actions for byte-compiling, package-lint, checkdoc, etc. this would be less of an issue.

FYI, Eask does that, but not the same standard as MELPA.

[port19x]

You're already catching non-package PRs via

    paths:
      - 'recipes/**'

[jcs090218]

You're already catching non-package PRs via

Not good enough. You can change the recipe and other file in the same PR.

[port19x]

Would it be sufficient to catch if anything else has been modified?
Also why would you do that?
Wouldn't only melpa maintainers and contributors ever touch sth outside of recipes?

[jcs090218]

Would it be sufficient to catch if anything else has been modified?

For example?

Wouldn't only melpa maintainers and contributors ever touch sth outside of recipes?

Yes, but we don't assume people won't, right?

[port19x]

It is possible to have a complimentary check that asserts the inverse of the recipes path, but I'm not sure how elegant that is.

Yes, but we don't assume people won't, right?

I would be in favor of a separation of concerns.
Can recipe authors decide to contribute to melpa beyond their recipe? definitely.
Should both the recipe contribution and the general contribution share the same PR? I believe not

[jcs090218]

It is possible to have a complimentary check that asserts the inverse of the recipes path, but I'm not sure how elegant that is.

Let me know if you figure it out! :)

I would be in favor of a separation of concerns.
Can recipe authors decide to contribute to melpa beyond their recipe? definitely.
Should both the recipe contribution and the general contribution share the same PR? I believe not

Sorry, I don't think it's worth discussing this since it only requires me to delete a few lines. You have your point, and I see it; I used to have the same perspective as you. I'll agree to disagree since it's not worth the effort!

[riscy]

Hi @jcs090218 - I apologize for having not looked at this yet, most of my MELPA bandwidth is going into reviewing recipes at the moment.

I'll give this the closer look it deserves, but I will note that I've also been hesitant about melpazoid's reliability for reviewing recipes in the general case. For a lot of simple recipes it works. For a lot of nontrivial recipes I'm making a fix here or there, or discovering that some of the output is incorrect and needs to be hand-edited before being passed to the author as "advice".

[jcs090218]

I'll give this the closer look it deserves, but I will note that I've also been hesitant about melpazoid's reliability for reviewing recipes in the general case.

Yeah, I agree. There are a few things need to improve to make it perfect.

For a lot of simple recipes it works. For a lot of nontrivial recipes I'm making a fix here or there, or discovering that some of the output is incorrect and needs to be hand-edited before being passed to the author as "advice".

I'm not too worried about this since many big repo are doing things like this. (1) They simply put a note in the comment, for example "This is an automated test, please ignore it if it doesn't look right. There will soon be a maintainer to review it!", etc. Of course, you can put any note you want. (2) Even if something went wrong, you can always change review-pr.yml (this PR) or melpazoid to improve the result since you are the maintainer of MELPA and author of melpazoid.