feat: allow "insert" of special strings

[gierdo]

Allow the insertion of special strings, e.g. the creation of sections or addition of comments in the generated CODEOWNERS file

[andrewring]

@gierdo This is interesting, and I'm guessing related to GitLab CODEOWNERS format?

In general, it seems fine, but assuming I'm correct, I'm wondering what this looks like at a larger scale. If I'm skimming the documentation correctly, it looks like once you open a section, you stay there until you open a new one, or end the file. If that's the case, you could easily end up with new files being added somewhere lexigraphically later, and the newly generated CODEOWNERS might result in the new code being considered in the section.

So if we take this example, we have

# /OWNERS
insert # This repository is maintained by the platform team
insert ^[DevOps] @org/devops-team
user0
user1

you get this as you noted

# This repository is maintained by the platform team
^[DevOps] @org/devops-team

* @user0 @user1

But lets try another scenario. You have a tree like this:

/
├── apps/
│   └── backend/
│       └── OWNERS  <-- Contains: [Backend] @team-backend
└── libs/
    └── auth/
        └── OWNERS  <-- Contains: /libs/auth/ @team-auth

so now we have a CODEOWNERS file that looks like:

# Generated CODEOWNERS

# From /apps/backend/OWNERS
[Backend]
/apps/backend/ @team-backend

# From /libs/auth/OWNERS
# BUG: Because no new section was started here, 
# this rule is now implicitly part of the [Backend] section!
/libs/auth/ @team-auth

If that is, in fact, the intended use, I think a safe implementation would need to make sections a first class citizen, and cluster things in the same section together after all of the other, non-sectioned owners rules. I think there might be some complications if you re-use names, but I'm not really familiar enough with GitLab to have a clear understanding of the implications or possible solutions.

That doesn't necessarily need to be resolved in order for this change to go in, but I think if it's going in with this type of example, we would want to, at a minimum, call out this issue.

[gierdo]

Yes, you're spot on, it's for GitLab's CODEOWNERS.

The issue is also valid for nested OWNERS structures etc, and not necessarily an issue, e.g. if only used for comment insertion.

We're "solving" the issue by accepting it and not auto-committing the generated CODEOWNERS file, but opening a merge request if there is a change in the CODEOWNERS file.

If you are okay with it, I will document the behaviour and add a warning that creating sections may have unintended effects.

[perfinion]

Hmm, I like the feature but am a bit worried about allowing arbitrary insertions in any OWNERS file ending up in /.github/CODEOWNERS.

lets say you originally have this as the codeowners:

/OWNERS file giving all ownership to @devops
/app1/OWNERS with inherit=false, giving ownership to @team1 
/app2/OWNERS with inherit=false, giving ownership to @team2

Now changing /tools/build.sh requires devops team approval. and /app2/hello.py requires team2

you could do:

1. person on team1 adds insert /app2/** @team1 to /app1/OWNERS
2. Merge that commit, it would not require approval from devops or from team2.
3. team1 can now cause chaos in the build script and app2, all without approval from either team

Maybe limit the inserts only to the toplevel /OWNERS or /.github/OWNERS ?
I like the feature, I've wanted to have a rule like /**/BUILD.bazel @devops but that did not work because the more-specific lines in codeowners meant putting that in the top level does not apply. I'm just worried about non-toplevel.

[gierdo]

Hmm, I like the feature but am a bit worried about allowing arbitrary insertions in any OWNERS file ending up in /.github/CODEOWNERS.

lets say you originally have this as the codeowners:

/OWNERS file giving all ownership to @devops
/app1/OWNERS with inherit=false, giving ownership to @team1 
/app2/OWNERS with inherit=false, giving ownership to @team2

Now changing /tools/build.sh requires devops team approval. and /app2/hello.py requires team2

you could do:

1. person on team1 adds insert /app2/** @team1 to /app1/OWNERS
2. Merge that commit, it would not require approval from devops or from team2.
3. team1 can now cause chaos in the build script and app2, all without approval from either team

Maybe limit the inserts only to the toplevel /OWNERS or /.github/OWNERS ? I like the feature, I've wanted to have a rule like /**/BUILD.bazel @devops but that did not work because the more-specific lines in codeowners meant putting that in the top level does not apply. I'm just worried about non-toplevel.

I haven't thought of your case, but you're right.
We don't have the issue, because the generated CODEOWNERS have to pass a review.

Would you be okay with a flag to allow non-toplevel insertions?

[perfinion]

We don't have the issue, because the generated CODEOWNERS have to pass a review.

oh, yeah we have no codeowners on the codeowners file itself and rely on the pre-commit check making sure there is no change in the file other than with this tool.

Would you be okay with a flag to allow non-toplevel insertions?

Yeah that'd be fine with me. Not sure how it should be structured tho, that is probably up to @andrewring. a few options that come to find:

1. the top-level OWNERS could double as a config file, set already ahve set inherit = so maybe set child_insert = false ?
2. a commandline arg, and plumb it through from the pre-commit configs, then set it there.

Since you have more use-cases for it, are non-toplevel inserts something you'd always want everywhere? or would it be something to restrict to only specific dirs? (ie do we just want a true/false flag or should it be some kind of --allow-inserts=/tools,/.github to only allow it in certain dirs?)

[andrewring]

I definitely want to tread carefully in this case. The recommended use is not to have an owner for the CODEOWNERS file, since part of the intent is to avoid a review bottleneck.

Adding comments would be simple enough. I'm not sure what the primary use case is, given the real values are in the OWNERS files, but it's harmless in any event. We could put something like export-comment I'm a comment, or something, and auto prepend the # to it.

As for the arbitrary insertion, I'm wondering if we could reduce the scope and still meet your current needs. This is, to some extent, kicking the can down the road. If it does what you need, though, I think it would significantly reduce the risk, and avoid needing to do more serious analysis on the implications. We could add special values in the repo root OWNERS file that allow you to specify an arbitrary header and footer, for example. Would that be sufficient, @gierdo?

[gierdo]

I definitely want to tread carefully in this case. The recommended use is not to have an owner for the CODEOWNERS file, since part of the intent is to avoid a review bottleneck.

Adding comments would be simple enough. I'm not sure what the primary use case is, given the real values are in the OWNERS files, but it's harmless in any event. We could put something like export-comment I'm a comment, or something, and auto prepend the # to it.

As for the arbitrary insertion, I'm wondering if we could reduce the scope and still meet your current needs. This is, to some extent, kicking the can down the road. If it does what you need, though, I think it would significantly reduce the risk, and avoid needing to do more serious analysis on the implications. We could add special values in the repo root OWNERS file that allow you to specify an arbitrary header and footer, for example. Would that be sufficient, @gierdo?

Yes, I understand your concerns.

Our main usecase is specifying the number of required codeowner approvals in the default section. So, this is what the root OWNERS would look like.

The usecase could be implemented just fine with the arbitrary header solution.

# Default Code owners, matching all files that are _not_ included in other rules.
# Inherited by nested OWNERS definitions

insert [CODEOWNERS][1] # Require at least one codeowner approval

@@maintainer
@@owner

[andrewring]

Our main usecase is specifying the number of required codeowner approvals in the default section. So, this is what the root OWNERS would look like.

The usecase could be implemented just fine with the arbitrary header solution.

# Default Code owners, matching all files that are _not_ included in other rules.
# Inherited by nested OWNERS definitions

insert [CODEOWNERS][1] # Require at least one codeowner approval

@@maintainer
@@owner

Great! Would there be additional value for you if we included the ability to add comments to the generated CODEOWNERS file via the OWNERS files? As noted in the documentation here, the general use is expected to not have owners on the CODEOWNERS file, so it wouldn't necessarily have a lot of value in those cases, but I'm not sure for yours. I suppose if you have someone who's trying to just look in CODEOWNERS to not navigate the tree, having comments could have some value. For that, we could implent a shorthand form so you could do something like

# I'm a regular, local comment
#^ I'm a special comment that gets promoted up into the CODEOWNERS file!

Thoughts?

[gierdo]

Our main usecase is specifying the number of required codeowner approvals in the default section. So, this is what the root OWNERS would look like.
The usecase could be implemented just fine with the arbitrary header solution.

# Default Code owners, matching all files that are _not_ included in other rules.
# Inherited by nested OWNERS definitions

insert [CODEOWNERS][1] # Require at least one codeowner approval

@@maintainer
@@owner

Great! Would there be additional value for you if we included the ability to add comments to the generated CODEOWNERS file via the OWNERS files? As noted in the documentation here, the general use is expected to not have owners on the CODEOWNERS file, so it wouldn't necessarily have a lot of value in those cases, but I'm not sure for yours. I suppose if you have someone who's trying to just look in CODEOWNERS to not navigate the tree, having comments could have some value. For that, we could implent a shorthand form so you could do something like

# I'm a regular, local comment
#^ I'm a special comment that gets promoted up into the CODEOWNERS file!

Thoughts?

The comment feature is not required, I think. It would be more of a convenience feature without real added value, also in our usecase.

About expectations and who-does-what, I can try to implement the proposed solution, but I'm certain that somebody else could do it faster and cleaner 😆

Do you already have a solution in mind, or should I give it a shot?

[andrewring]

The comment feature is not required, I think. It would be more of a convenience feature without real added value, also in our usecase.

About expectations and who-does-what, I can try to implement the proposed solution, but I'm certain that somebody else could do it faster and cleaner 😆

Do you already have a solution in mind, or should I give it a shot?

@gierdo Sorry for going silent on you, I missed this 😬

To review, I think we're now saying we want to have 1) a method to prepend arbitrary contents at the top of the generated CODEOWNERS file, which can be controlled in the root OWNERS file, and 2) optionally a method to embed comments in arbitrary places to be added to the generated CODEOWNERS file above the next rule. Value of 2 is minimal, and so may be omitted.

For 1, I think that should be fairly straightforward, as it won't need to be aggregated across the tree. We can just use something like the #^ notation above, or a single fenced section at the top of the one OWNERS file. I'm a little short on time at the moment, so if you have cycles to give it a shot, I'll be happy to take a look. :)

[gierdo]

The comment feature is not required, I think. It would be more of a convenience feature without real added value, also in our usecase.
About expectations and who-does-what, I can try to implement the proposed solution, but I'm certain that somebody else could do it faster and cleaner 😆
Do you already have a solution in mind, or should I give it a shot?

@gierdo Sorry for going silent on you, I missed this 😬

To review, I think we're now saying we want to have 1) a method to prepend arbitrary contents at the top of the generated CODEOWNERS file, which can be controlled in the root OWNERS file, and 2) optionally a method to embed comments in arbitrary places to be added to the generated CODEOWNERS file above the next rule. Value of 2 is minimal, and so may be omitted.

For 1, I think that should be fairly straightforward, as it won't need to be aggregated across the tree. We can just use something like the #^ notation above, or a single fenced section at the top of the one OWNERS file. I'm a little short on time at the moment, so if you have cycles to give it a shot, I'll be happy to take a look. :)

Thank you for your reply!
Of course, now I missed it 😆

I'll try to give it a shot.