[owners] implement blocking Travis check to validate modified OWNERS.yaml syntax about amp-github-apps HOT 11 CLOSED

We could probably make this a blocking check rather than just a warning; I don't see much reason we'd want to allow a broken/empty OWNERS file to be merged

from amp-github-apps.

As I've been working with the OWNERS files and parsing, there's a lot of cases where incorrect YAML could totally break parsing without being obvious. I think either A) a test suite should be added to explicitly test that all OWNERS files can be parsed without error, and block PRs which try to submit breaking changes to OWNERS files or B) a standalone check (probably also run by the OWNERS bot) checks any modified OWNERS files and blocks the PR if it introduces breaking changes.

I think B is probably the best option, especially since it could live in the existing owners bot app and just add a separate check-run to PRs altering OWNERS files. This would become more valuable as we expand the supported syntax of OWNERS files to include wildcards, teams, file-level rules, etc. If we do it this way, once we fix any existing issues in OWNERS files, we wouldn't need the warnings as we would have a guarantee that all OWNERS files comply with the spec, and the owners bot itself wouldn't have to handle as many edge/error cases.

from amp-github-apps.

Agree that a check of this sort is best run by the owners bot, eventually in blocking mode. +1 to B in the comment above.

from amp-github-apps.

I'm of the other opinion - I think that we should add a blocking check on the amphtml repository's Travis runs that verifies nobody breaks the ownership model by verifying that all OWNERS.yaml files are correct. Just like we don't allow merging .js or .json files!

That way the owners bot is guaranteed* to work with correct data. Trying to recover what are the correct rules from a set of incorrect files can open us up to troubles

*to the extent that anything in software is guaranteed

from amp-github-apps.

I leaned towards the bot partially because I think it makes sense for the repo with the owners bot to also define the checks. Otherwise, a change could be made to the owners bot/parser and that would require a separate update to tests in Travis. The risk of these getting out of sync across repos seems unweildy. Whereas if they use the same parser to syntax-check as to parse, it ensures synchronicity. We already need to be running the owners bot on every PR, and it makes sense to be that it should be responsible for verifying and/or blocking OWNERS.yaml change. The parser already supports tracking and reporting errors in syntax or rule definitions, so all the owners syntax check would have to do is see if any error was encountered and block the PR with all errors if so. This seems a lot simpler than creating an implementation of a syntax checker in a separate repository from where the syntax is defined. Plus, it allows us to use the OWNERS bot/syntax checker on multiple repos, instead of just amphtml

from amp-github-apps.

I'm with @danielrozenberg that the syntax check should be a Travis check.

I understand that having the same parser in 2 repos is unwieldy, but we already are dealing with 2 repos with having the OWNERS.yaml files living in amphtml. It makes sense that amphtml checks the syntax of its own files, and we have determineBuildTargets() that can cleanly detect an OWNERS build target.

A problem with using the bot to check syntax, is that someone could go around the owners bot and add themselves as an owner. The owners bot should only read from master, not PR changes. Using Travis as a gate check prevents this.

from amp-github-apps.

The 'already dealing with 2 repos' bit doesn't really apply; we want OWNERS.yaml in potentially many repos, not just amphtml.

The other problem is not actually applicable. The owners bot reads from master for determining which changes are acceptable. The syntax check pulls from the PR.

from amp-github-apps.

Which other repos? amphtml is large, with 3k+ forks, so the use case for having an owners bot makes sense. Not sure about other repos which are smaller, where I imagine the contributors generally know who to select as a reviewer.

from amp-github-apps.

Part of the design goals behind this and other apps is to make them useful and reusable on other repositories, both within and outside of AMP. This is why the org and repo names are never hard-coded. There are extensions with many contributors (Wordpress comes to mind) and in conversations I've had other developers comment on how the owners bot logic would be useful. It's not just about "knowing who to select as a reviewer"; it is also about enforcing ownership and reviews, auto-assigning reviewers, and notifying users who need to follow changes in given file paths and patterns. I've already had two apps/projects in which supporting other repositories was not part of the original design, but the way the apps were structured made it relatively painless to reuse. Since that time, both of those projects have been asked about by other projects or repos wanting to reuse that same functionality. I see no reason to develop this with the assumption that amphtml is the only repository that could use it; even this repository could utilize it, and there have been discussions around enabling it here.

I maintain that fragmenting Travis checks and parsing syntax across any projects that want to support OWNERS adds additional complexity and little to no benefit, while this bot has clear ownership the OWNERS rules and syntax and will already be monitoring updates to any PR that might affect the OWNERS.yaml files.

from amp-github-apps.

Interesting discussion, with valid arguments on both sides.

When I read this phrase in the initial problem statement print a warning on the "checks" tab for it to be fixed, I thought it was a simple case of the owners bot being the right place to do so. This discussion makes me realize there's more to it.

All these statements independently make sense:

• Trying to recover what are the correct rules from a set of incorrect files can open us up to troubles
• It makes sense that amphtml checks the syntax of its own files
• Someone could go around the owners bot and add themselves as an owner
• A change could be made to the owners bot/parser and that would require a separate update to tests in Travis

The scope of the OWNERS.yaml check appears to have expanded to:

1. Detect empty OWNERS.yaml files (original problem statement)
2. Detect errors in file format (what are the rules to check for?)
3. Detect if someone has incorrectly added themselves as an owner
4. Detect if a directory that needs an OWNERS.yaml is missing it (like a new extension?)
5. Decide which checks to run when a file PR (only?) contains changes to OWNERS.yaml files
6. Anything else?

Would it make sense to do parts of this on the owners bot (2) and other parts on Travis (1, 3, 4, 5), thereby separating the stuff the respective repos care about and allowing for the checks to be changed independent of one another?

Or maybe this topic deserves an in-person discussion? 😃

@rcebulko @danielrozenberg @estherkim WDYT?

from amp-github-apps.

1. I see this as being in the same bucket as an error in file format
2. These are errors the parser already raises. Notably, these are not just syntax errors; they are also semantic errors, such as invalid teams. The parser would have to be re-implemented/made available to every repo that wanted to write tests. As would the logic for validating teams.
3. I don't think this is relevant; it's fine if someone adds themself as an owner, because that addition would only be allowed if that PR gets owners approval. Meaning, I could add myself to foo/OWNERS.yaml, but I won't be able to submit that unless someone higher up in OWNERS.yaml says it's okay. So, not a necessary check.
4. I don't think this is a need. It is expected for many directories not to have an OWNERS file.
5. I'm not clear on what this means.
6. Not that I'm aware of.

My feeling is that 2 does indeed belong in the owners bot, while the remaining (1, 3, 4) are not actually needed. I'm unclear on 5.

from amp-github-apps.