Added github check for accidental assignment submission

[adamblanchard]

Context: Slack discussion here

The idea is to create some kind of automation to help guide trainees who accidentally make a PR to the parent repo, instead of their own fork. This removes the need for a mentor or staff member to manually comment, and gives the trainees a chance to fix the issue on their own.

Notes and assumptions:

1. This check is only based on whether the fact the PR comes from a fork or not. In that case, I assume it's probably a trainee.
2. I assume most mentors/legit contributors will be part of the hyf org and clone the repo rather than fork.
3. Even if a mentor forks and contributes a change, it will be significantly less often than the number of trainees who are submitting prs, so we can deal with that on a case by case basis.
4. I've tested this on a private org and repo I have. It appears to work as intended.

I tried to create a check to see if the author was in the "curriculum crew" group or in the hyf org. Unfortunately, that appeared to need greater permissions for forks to be able to read our org info. I just stopped at that point, considering I believe the check below is sufficient enough for now.

What i need a review for:

1. This is the first github action I've ever written, so if you are more experienced and have suggestions please share
2. If there is a better check vs the fork idea, please help test it and share :)

[rvedotrc]

I suspect that all of this could replaced by something like:

Suggested change

	- name: Check PR is a fork
	id: check_fork
	uses: actions/github-script@v7
	with:
	script: |
	const pr = context.payload.pull_request;
	const isFork = pr.head.repo && pr.head.repo.id !== pr.base.repo.id;
	console.log(`From fork? ${isFork}`);
	core.setOutput('should_comment', isFork);
	- name: Comment assignment instructions for trainees
	if: steps.check_fork.outputs.should_comment == 'true'
	- name: Comment assignment instructions for trainees
	if: pull_request.head.fork == 'true'

but I'm not sure of the exact syntax at this exact moment. Would be nice if it could be simplified, though.

[adamblanchard]

I originally used pull_request.head.fork and i couldn't get it to work. I can check my notes later to see if I can remember why. But happy to simplify, for sure!

[rvedotrc]

Given the requirements here (i.e. very simple: just append a comment), I think that this could be simplified to something like:

Suggested change

	uses: thollander/actions-comment-pull-request@v3
	continue-on-error: true
	with:
	message: |
	run: |
	gh pr comment --body-file /dev/stdin <<COMMENT

(and with an appropriate COMMENT end marker)

which, because of not having the extra dependency, would have the benefits of being slightly more efficient, and more secure.

[rvedotrc]

I had a play with this.

Resulting code: https://github.com/rvedotrc/no-fork-test/blob/main/.github/workflows/assignment-check.yml

In action (PR from fork to original): rvedotrc/no-fork-test#13

In action (PR from original to original): rvedotrc/no-fork-test#14

The differences:

• moved the "if" check to the job, so that there's no need even to get a runner if we're not in the "fork" case
• changed the check from "HEAD repo != BASE repo" to "HEAD repo is a fork, BASE repo is not a fork". Which only makes a difference in the highly-edge case of a PR from the original repo, to the fork. i.e. the opposite to the usual.
• use the provided gh command line tool to add the comment (for which we then need to pass the repo full name, the PR number, and the GH token).

[shpomp]

@adamblanchard @rvedotrc
I had a very simple idea, maybe overly simplistic? 🙈 #157

I do not get a sense that there is a point to check whether submission is coming from a fork or not.
I have described my primitive logic briefly in the PR. #157

[adamblanchard]

Thanks both @rvedotrc @shpomp for exploring this too.

The idea behind the fork check is the simplest way to assume if it is an assignment or not. All trainees need to submit via their fork, so this check will run in that case. If it is against the base repo (e.g. our template) they did it wrong and need a comment to help.

We're also assuming that anyone who wants to actually contribute to the template (e.g. staff, mentors), will very likely be in our org and most likely clone it (e.g no fork). In the case that a mentor does fork and contribute, they also get a little prompt at the bottom of the message to reach out. They should be in our org/curriculum-crew anyway, so it's not a bad point to prompt them.

Also, the number of times trainees make these PRs is like 45 a week, for three teams. The times a mentor might make a PR in the first place, never mind from a fork, is very very small in comparison. So i'm fine with this solution mostly taking care of the trainee case.

Looking through all three of our ideas, I think @rvedotrc you have simplified it amazingly 👏 , and we should go with this route for now as a solution for the immediate challenge. I will update this PR with your solution.

New ideas welcome to be iterated on afterwards, as always👌

[adamblanchard]

Okies, this is ready for re-review.

The only thing i changed was the name of the action, since it was a bit misleading to see it skipped while being called "assignment check" since it did actually check if it was an assignment already.

[rvedotrc]

Probably a good idea to rename the yml file to match

[rvedotrc]

Suggested change

	(If you are a mentor, and are trying to contribute, reach out in #mentorroom for help.)
	(If you are a mentor, and you are trying to contribute, then please reach out in #mentorroom for help.)

Minor easing of readability.

[rvedotrc]

Every time I read this, I stumble at the clause containing two negative words: both "incorrectly" and "wrong". I wonder if we can find a clearer wording?

[shpomp]

Looks great!

I am only wondering why you would not close the PR @adamblanchard?
The way I see it is that the end result is the same either way, but force closing it rips the plaster and does not require extra step from the trainee.
I worry that leaving it be just calls for the PR to be left hanging there forever.
Although we can observe the workflow running live and maybe it will prove me wrong :)