Actions: Add change note checker

[tausbn]

Checks whether a change note is present on a PR (assuming it should be) and adds a label to indicate when its not.

Considers all .ql and .qll files relevant, unless:

• the files are in a subdirectory of experimental, or
• the PR has been labelled with the label "Minor change"

If any relevant files are found, and there is no corresponding change note, the label "Needs change note" is applied to the PR.

I'm thinking we may want to roll this out to just the Python team at first. I have tested it manually, but I can't guarantee that it works perfectly.

Open questions:

• Should this add a comment instead of a label?
• Should this fail the action and block merging?

cc: @RasmusWL @calumgrant @adityasharad

[aschackmull]

I think we had something similar somewhere that worked like so: Anything without a change note that looks like it might need a change note (based on changed file paths, I guess) cannot be merged without a manually added label "don't-need-change-note" or something like that.
I think that seems like a slightly better workflow?

[aschackmull]

So I guess what I'm saying is that, yes, I think this should block merging, and that the "Minor change" label ought to be renamed to something including "change note" to make it clear what it means to apply that label.

[tausbn]

So I guess what I'm saying is that, yes, I think this should block merging, and that the "Minor change" label ought to be renamed to something including "change note" to make it clear what it means to apply that label.

I was planning on putting this in the description of the label. I think "Doesn't need change note" is too long for a label.

[aschackmull]

https://github.com/github/codeql-go/labels uses a "no-change-note-required" label.

[aschackmull]

Possibly a good idea to check with the Go team how exactly they're using these change-note-related labels?

[tausbn]

Note to self: we should also exclude the test directories. Those things usually don't need change notes.

[RasmusWL]

Nice 💪

Should this fail the action and block merging?

yes

https://github.com/github/codeql-go/labels uses a "no-change-note-required" label.

I would like a more clear name for the label than "Minor change" as well, so I'm all for using the "no-change-note-required" as well 👍

[tausbn]

Any thoughts on feedback for the user? Is adding a label sufficient, or would it be better for it to add a comment instead?
(The problem with comments is that it'll keep adding comments for every change until you either add the no-change-note-required label or add the change note.)

[aschackmull]

CI fails for lots of reasons, so as long as you can click on the failed check and see why, then I don't think the PR needs any sort of feedback.

[RasmusWL]

Any thoughts on feedback for the user? Is adding a label sufficient, or would it be better for it to add a comment instead?
(The problem with comments is that it'll keep adding comments for every change until you either add the no-change-note-required label or add the change note.)

I agree with @aschackmull, if the CI check fails, that is my expected level of feedback for the user (like the QLdoc check)

[tausbn]

Alright, in that case one more thing needs to be decided: when should the check trigger?
Currently it'll trigger on the default set of events (opened, reopened, and synchronize), and it'll definitely also need to trigger on labeled and unlabeled to handle the no-change-note-required label.

However, I think if it is to fail the CI, then it would be bothersome to already trigger when the PR is created. Imagine I'm creating a PR early in the process, before I even know what the change note should say. I think it would be bothersome for the CI to fail outright, since that'll mean extra effort in order to figure out whether the CI failure is due to test issues (which is very relevant at that stage) or due to a missing change note (which is less relevant).

In other words, I'm leaning towards only triggering when the PR is not in draft mode.

[aschackmull]

In other words, I'm leaning towards only triggering when the PR is not in draft mode.

Sounds good to me.

[adityasharad]

Note this makes this workflow security-sensitive, because it'll run on PRs from forks but in the context of the base repo and branch. That's what you need here, but be extra careful not to use data from the event payload (PR body, etc) in the body of a script.

[tausbn]

Hmm... So I am using some data from the payload (to wit, the list of files contained therein), but I guess that might be okay.

[adityasharad]

I think that's ok, especially as it's coming back from the API rather than directly from the event payload.

[adityasharad]

I think that's ok, especially as it's coming back from the API rather than directly from the event payload.