Add support for CODEOWNERS

[marshall007]

Fixes #9474

This currently depends on hairyhenderson/go-codeowners. Please let me know if you would prefer to incorporate the implementation into this repo or in bep/gitmap.

Even though it has no explicit dependency on git, this is behind the same enableGitInfo config option.

I have not updated all the markdown files under docs/. I couldn't figure out if these were auto generated or not.

Cheers!

[CLAassistant]

All committers have signed the CLA.

[marshall007]

FYI @bep

[bep]

Thanks for this;

I don't mind adding a new dependency, but I had a look at the code and it walks from the root and upwards until it finds a CODEOWNERS file. We have been very strict in Hugo in that we don't read files outside of the defined set of directories, and I don't see how this could be wanted behaviour, either.

• https://github.com/hairyhenderson/go-codeowners/blob/master/codeowners.go#L37

I see there's a FromReader variant that you could consider.

Other than that this looks fine. I will need to do some cleanups in this area, but that's on me.

[marshall007]

@bep thanks, I have adjusted our implementation to only look for CODEOWNERS files at the conventional paths (., docs, .gitlab, and .github) from the root of the project as defined by Hugo's workingDir.

I think the current implementation (and my original one) is technically incomplete because some providers support overriding CODEOWNERS at every directory level. A full implementation would require that we find the closest CODEOWNERS file to the current page that has a match.

I think just looking at the conventional paths off the root is good enough for now and we can do something more complex later on if there is demand for it.

WDYT?

[jmooring]

I think as soon as you start looking at anything other than the conventional paths off the root, things get messy. For example, a logical content directory that consists of a physical content directory with the addition of a couple of mounted content modules.

[marshall007]

@jmooring I agree with you. The use case you cited regarding mounts is a particularly compelling reason to explicitly never support anything other than conventional paths off the root.

Not only would anything else be messy from an implementation perspective, but I don't think users would expect for remote CODEOWNERS to override what they have specified at the root of their own project. Not to mention that the remote may not even reference the same host, making the usernames meaningless.

[jmooring]

@marshall007

In the array of returned codeowners, can you only include those elements beginning with @? Returning users and teams (both beginning with @) is fine, but the CODEOWNERS file can also include email addresses.

If I have a private repository, and I use a theme that leverages the CODEOWNERS data, I could inadvertently expose email addresses that should be kept private.

There may be other privacy/security concerns here, but this is the first one that's occurred to me.

EDIT: Thinking through this a bit more, would it wise to enable access to the CODEOWNERS data separately from the enableGitInfo setting?

[jmooring]

@marshall007 While the filename is a single word (CODEOWNERS), the GitHub and GitLab documentation refer to "code owner" (singular) and "code owners" (plural). To be consistent with other page methods, it seems like "CodeOwners" might be a better choice than "Codeowners". E.g.,

{{ with .CodeOwners }}

[marshall007]

If I have a private repository, and I use a theme that leverages the CODEOWNERS data, I could inadvertently expose email addresses that should be kept private.

@jmooring this is no different than the current .GitInfo object exposing commit author metadata (which includes email). If we are opinionated in parsing the individual owners it limits the use cases. If you are using emails in your CODEOWNERS file then this makes the feature arbitrarily useless to you. I think this is a good reason to keep it behind --enableGitInfo though because the privacy implications are the same.

To be consistent with other page methods, it seems like "CodeOwners" might be a better choice than "Codeowners".

I have no strong opinion here but I am so accustomed to seeing CODEOWNERS as one word that the pascal casing looks strange to me.

[bep]

I will merge this once the test passes; I may change the name of the method before I release, though, but I think it makes sense to have this under one flag -- theres certainly no broader privacy concerns for the CODEOWNERS compared to the GitInfo object.

[marshall007]

@bep can you rerun the tests again? I don't think the failure is my fault. Looks like a timeout on fetching a dependency.

[jmooring]

@marshall I spent some time last night figuring out how I would use this, and it works really well. Thanks!

Here's an example that retrieves and displays the code owners' information from GitLab and GitHub:

git clone --single-branch -b hugo-github-issue-9529 https://github.com/jmooring/hugo-testing hugo-github-issue-9529
cd hugo-github-issue-9529
hugo server

Creating the partial for this was a non-trivial exercise, and while doing so it occurred to me that there is a difference between what is exposed in a page's .GitInfo object and its .Codeowners object:

• The .GitInfo object contains information about who did what. It is intentional.
• The .Codeowners object contains information about who we're going to notify; the information is arbitrary, not tied to any commits, and may be incorrect.

My concern here is not about bad actors (one could use os.ReadFile for that), but instead about inadvertent exposure of email addresses that are not related to any of the commits.

I'm fine proceeding as is, but I did want to take the opportunity to fully describe my concern.

Thanks again for this; works great!

[marshall007]

@jmooring thanks, I really appreciate you taking the time to produce such a detailed example! Your codeowners.html partial aligns with what I was planning to implement in our own project (but yours is way more robust).

Perhaps as a followup we could consider adding a built-in shortcode for this? Recommending use of the shortcode might be a good way to guard against exposure of email addresses without limiting the core implementation for more advanced/internal use cases.

[bep]

The .Codeowners object contains information about who we're going to notify; the information is arbitrary, not tied to any commits, and may be incorrect.

Sure, but that is the thing with lots of things in life. The CODEOWNERS file obviously needs to be maintained carefully to be of value.

[ghost]

@marshall007 While performing some code review for a hugo version upgrade, I noticed that the only CODEOWNERS definition in place for the software is the testsite directory. Could I confirm that that's as-intended?

[marshall007]

@jayaddison-collabora yes that is intentional. It is there to test the functionality of parsing CODEOWNERS files. It is not intended to be an authoritative CODEOWNERS file for the repository in general.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.