Add CI checks for italia/ hierarchical maintainership.

[bfabio]

When opening a PR changing something under the italia/ identifier, check whether the author is listed as a maintainer in one of the READMEs, starting from the top directory (italia/) and and progressively narrowing down to the specific directory of the change.

Then create a comment with the findings, tagging the relevant users.

This is a PoC for #3799 (comment)
cc @dgarijo

[davidlehn]

Nice! Automating maintainer checks has been on the todo list forever. As mentioned, this will be more of a guideline than a strict rule in every case.

There's another check action here: #3786

As said in that PR, I was thinking of a config file to setup this sort of metadata. A structured data file would be far easier to parse, validate, and use than README parsing and heuristics. Adding maintainer info would then be straightforward. But there are concerns with duplication and syncing of info, since people seem to like to use READMEs. Some design experimentation is likely needed.

I'll have to look at this script more. I'm not that familiar with their API but this looks easier to write than I imagined.

Does the github.rest.repos.getContent call for the README get the PR readme or the target branch readme? Need to handle the use case of maintainers being added and removed in a good way. At a minimum handle the case where PR author adds themselves which would need approval. Probably good to make notifications for any changes to important data like that.

I do think we'd want this to be more generalized before being added. But it seems a great start!

[bfabio]

Nice! Automating maintainer checks has been on the todo list forever. As mentioned, this will be more of a guideline than a strict rule in every case.

There's another check action here: #3786

@davidlehn great, I'm glad we're all on the same page about automation!

As said in that PR, I was thinking of a config file to setup this sort of metadata. A structured data file would be far easier to parse, validate, and use than README parsing and heuristics. Adding maintainer info would then be straightforward. But there are concerns with duplication and syncing of info, since people seem to like to use READMEs. Some design experimentation is likely needed.

I'll have to look at this script more. I'm not that familiar with their API but this looks easier to write than I imagined.

The rationale for using READMEs was avoiding duplication, as you mentioned, and keep things simple. I agree that for this PoC the regexp approach is a little rough, but I was thinking about a microformat (not in the sense of microformats.org, which AFAIU are tied to HTML, and I'd rather not add an HTML parser into this) to have both something more solid to parse and keep it readable in the READMEs.

Something like, for example:

• maintainer:https://github.com/abc123 or
• maintainer:@abc123
• or something equally simple

We can force it to be one per line, wdyt?

Also, these are examples of the output of this check:

• PR from a "maintainer"
• PR from non-maintainer

It checks each directory recursively, starting from the top-level one and going down to the more nested ones, to see if the maintainer is listed in any of the READMEs.

Note that in the second PR, the @ tags are not there so as not to notify users for this test, but they'll be included in this PR's version.

Does the github.rest.repos.getContent call for the README get the PR readme or the target branch readme?

It should be the target branch, but good catch. This is crucial for the whole process and we should double check it does what we mean.

Need to handle the use case of maintainers being added and removed in a good way. At a minimum handle the case where PR author adds themselves which would need approval. Probably good to make notifications for any changes to important data like that.

The action doesn't fail, but instead comments on the PR with a warning, as you can see from above, giving hints to the reviewer. In case a PR author adds themselves it would warn about them not being in one of the READMEs and tag the appropriate people, who can then review the PR.

I do think we'd want this to be more generalized before being added. But it seems a great start!

Nice to hear! The idea is that we can maybe operate on an opt-in basis, adding the toplevel dir of projects that want to do this check.

After that, the only place to generalize is https://github.com/perma-id/w3id.org/pull/3801/files#diff-d9ac58060f3a6f15a3f7e4744d195da1ecf3d3352fcefb89312357a980ddaebcR21

[bfabio]

Suggested change

	`Author @${author} is not listed in any README.md within the \`italia/\` directory`,
	`Author @${author} is not listed in any README.md within the \`${ROOT_DIR}/\` directory`,

[bfabio]

This is one of the most annoying missing features in GitHub actions: workflows from forks use a GITHUB_TOKEN with no permission to comment on the PR and every solution is either convoluted, a foot-gun or both.

pull_target_request is the simplest foot-gun and is not a security issue as long as:

• we don't checkout of the repository's code
• there are no secrets (https://github.com/perma-id/w3id.org/settings/secrets/actions)
• changes to this file are carefully reviewed, making sure the above items don't change over time or in the proposed patch