Thanks Paul,
Do you have some indication of when did this break?

BR
Alberto

On Fri, 26 Apr 2019 09:25:17 +0000
"Alberto Escolar Piedras (ALPI)" <ALPI@oticon.com> wrote:

Thanks Paul,
Do you have some indication of when did this break?

Well, I was on vacation for two weeks before Apr 15. It definitely
worked before that. And I saw first problematic case last week, after
returning from vacation. No idea if that might be related to
https://github.com/zephyrproject-rtos/zephyr/commit/636a7af43e704e25af29b31d565399fc02fef408
- nothing wrong is seen here.

Actually, fishing for that link, I see that a whole bunch of changes
was made to the CODEOWNERS file just recently:
https://github.com/zephyrproject-rtos/zephyr/commits/master/CODEOWNERS

Just imagine that any of those changes could break syntax a bit or
propagation of changes to it could glitch on Github side, leading to
the effect we see. And that's not counting a possible coincidence of
Github tighting up the syntax for that file...

BR
Alberto

-----Original Message-----
From: tsc@lists.zephyrproject.org
[mailto:tsc@lists.zephyrproject.org] On Behalf Of Paul Sokolovsky via
Lists.Zephyrproject.Org Sent: Friday 26 April 2019 11:10 To:
devel@lists.zephyrproject.org; tsc@lists.zephyrproject.org Cc:
tsc@lists.zephyrproject.org Subject: [Zephyr-tsc] Github CODEOWNERS
auto-assignment doesn't work as expected

Hello,

As was noticed in a few (multiple?) PRs, automatic reviewer
assignment based on the CODEOWNERS file no longer works as expected,
specifically fails to assign reviewers.

A recent example of this is
https://github.com/zephyrproject-rtos/zephyr/pull/15670 - while
there's an entry:

/subsys/net/ip/ @jukkar @tbursztyka @pfalcon

in CODEOWNERS, I wasn't requested for review. Again, that's not the
only case.

For a clean-room test, I submitted
https://github.com/zephyrproject-rtos/zephyr/pull/15683 and as can be
seen, the reviewer list is empty.

This is problematic, because there's a risk that under-reviewed
changes will get into the codebase, causing regressions. So, raising
to the awareness of the TSC.


--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs Follow Linaro:
http://www.facebook.com/pages/Linaro http://twitter.com/#!/linaroorg
- http://www.linaro.org/linaro-blog

--
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog

Hi,

I wasn't requested for review. Again, that's not the only case. [...]
This is problematic, because there's a risk that under-reviewed
changes will get into the codebase, causing regressions. So, raising
to the awareness of the TSC.

While github's features should really work as expected to save confusion
and everyone's time, I hope this "under-review" risk is very small
because I would expect every reviewer to... review the list of other
reviewers first thing and add anyone they think could be missing and
could help. Not just other co-owners, far from it.

Database-backed review tools like github avoid broadcasts. This can be
an advantage compared to pure email reviews but it requires a reasonable
"networking" effort; otherwise there's a risk of ending up with the
other extreme: semi-private, not very open-source reviews. I would also
expect developers to err on the "oversubscription" side a bit because
withdrawing from a review takes one click whereas you can't add yourself
to something you don't know exist.

The above is just based on experience with code reviews in general and
more specifically the acute problem of finding reviewers' time. Everyone
loves code reviews, yet for some reason I rarely ever see time scheduled
for them :-) Thanks in advance for correcting any of the generalities
above that wouldn't apply to Zephyr.

I see that a whole bunch of changes was made to the CODEOWNERS file
just recently.
Just imagine that any of those changes could break syntax a bit or
propagation of changes to it could glitch on Github side, leading to
the effect we see. And that's not counting a possible coincidence of
Github tighting up the syntax for that file...

It'd be nice from github to share the corresponding parsing code; think
open source.

Marc

On Fri, Apr 26, 2019 at 02:49 AM, Paul Sokolovsky wrote:

Actually, fishing for that link, I see that a whole bunch of changes
was made to the CODEOWNERS file just recently:
https://github.com/zephyrproject-rtos/zephyr/commits/master/CODEOWNERS

Just imagine that any of those changes could break syntax a bit or
propagation of changes to it could glitch on Github side, leading to
the effect we see.

Good guess. Anas found the bug (commas) and fixed it at https://github.com/zephyrproject-rtos/zephyr/commit/af3f81ef04bccd0b959

Curious how Anas debugged this because I just found this user who put a lot of effort:
http://www.benjaminoakes.com/git/2018/08/10/Testing-changes-to-GitHub-CODEOWNERS/

2018-08-31

Dear [github] support,

 

I’ve discovered what I believe to be a bug in CODEOWNERS. In particular, a single line failing to parse [trailing # comment] causes the entire file to stop identifying codeowners. This is in contrast to .gitignore, in which a single line failing to parse simply causes that line to be skipped. [...] I verified this behavior by making a sandbox repository and making pull requests until I narrowed down when the bug occurred [...] This situation was exacerbated by a lack of error messages in the GitHub UI. Ideally, we’d have access to a linter or at least highlighting in GitHub that would call out invalid syntax. 

Thanks for reaching out! This is currently expected behavior, but I can definitely understand why it would be unexpected. I’ll pass your feedback along to the team internally. I can’t say if or when a change will happen, but your feedback is in the right hands.

So apparently no "API" available, it would have been documented at https://help.github.com/en/articles/about-code-owners otherwise. No need to ask github either as the feedback is already "in the right hands".

Marc