tools: check_format verification that we add owners for new extensions by alyssawilk · Pull Request #7265 · envoyproxy/envoy

alyssawilk · 2019-06-13T19:20:04Z

Risk Level: Low (tools only, but my python is pretty terrible =P)
Testing: manual testing that new additions fail check_format
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2019-06-13T19:22:30Z

tools/check_format.py

Also if there's any of these we want to flag with owners while we're in here, that'd be great.

For most of these I can think of one maintainer owner but not a backup. I could also relax our checks and allow a single owner?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Awesome. Small question about common folders.

/wait

mattklein123 · 2019-06-13T20:47:34Z

tools/check_format.py

+  "extensions/filters/http/common",
+  "extensions/filters/http/common/aws",
+  "extensions/filters/http/squash",
+  "extensions/filters/common",


Can we maybe filter out directories with common in it? These are things that are shared by other extensions which should have owners?

We totally could. I think it's a bit nicer to list them explicitly so if we do code reviewer assignment we have explicit owners. We could arguably script around it but I think it's easier to have it explicit. WDYT?

Sure, no strong feelings on this if you think this is better.

Yeah, mild preference for this way, just for tooling.
Anything else you want to see here?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks, LGTM with a few small comment requests.

/wait

mattklein123 · 2019-06-17T16:33:53Z

tools/check_format.py

+
+  # Sanity check CODEOWNERS.  This doesn't need to be done in a multi-threaded
+  # manner as it is a small and limited list.
+  if dir_name.startswith('./source/extensions/') and '/' in dir_name[20:]:


Any way to make the 20 and 9 magic constants less magical either via some more descriptive code or some comments? It's not immediately clear to me what is going on here.

mattklein123 · 2019-06-17T16:34:36Z

tools/check_format.py

+    try:
+      with open("./CODEOWNERS") as f:
+        for line in f:
+          m = re.search(r'..*(extensions[^@]* )@.*@.*', line)


Can you add a human description of what the regex does for those of us that are regex challenged?

Done.
Note this does technically allow more than 2 owners (e.g. quic_listners) but does not allow 1 (reverse bridge filter) going forward.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123

Thanks, makes sense, but I think maybe a few more comments and it's failing format.

/wait

mattklein123 · 2019-06-17T17:56:05Z

tools/check_format.py

+
+  # Sanity check CODEOWNERS.  This doesn't need to be done in a multi-threaded
+  # manner as it is a small and limited list.
+  source_prefix =  './source/';


semicolons? 😉

mattklein123 · 2019-06-17T17:57:41Z

tools/check_format.py

+  # manner as it is a small and limited list.
+  source_prefix =  './source/';
+  full_prefix = './source/extensions/';
+  if dir_name.startswith(full_prefix) and '/' in dir_name[len(full_prefix):]:


sorry to come back to this but is the 2nd check just making sure it's a subdirectory? Can you add a small comment?

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

ipuustin · 2019-06-19T12:47:56Z

Hmm, this doesn't allow for arranging code in subdirectories inside extensions. I'm running in some trouble at #6326. Would the right solution be to have all files belonging to a single extension in one directory?

ipuustin · 2019-06-19T13:06:52Z

tools/check_format.py

+  # Check to see if this directory is a subdir under /source/extensions
+  # Also ignore top level directories under /source/extensions since we don't
+  # need owners for source/extensions/access_loggers etc, just the subdirectories.
+  if dir_name.startswith(full_prefix) and '/' in dir_name[len(full_prefix):]:


I'm quite late to the review but it seems that this could become more readable and portable if you used os.path functions as done elsewhere in this file. Maybe something like:

if os.path.dirname(dir_name) == "./source/extensions":

alyssawilk · 2019-06-19T13:14:59Z

This does allow for subdirectories in general. The problem is you're creating new sub-directories in an unowned directory, which is seen as creating a new unowned directory.

We can fix this one of three ways

adding the new directory into the known unowned directories
changing the format script to allow new unowned directories
tag the transport_sockets directory (and implicitly subdirectories) in CODEOWNERS

I'd prefer not to do 2) because if someone adds something overly broad like "extensions/filters/network/" to make the script shut up, we'd basically lose the owners checks for that branch of extensions. 1) is probably easiest but I'd prefer 3) since it gets us more owners.

I'd be inclined to add @PiotrSikora, and I'm checking with @envoyproxy/maintainers to see if anyone else is willing to claim ownership

ipuustin · 2019-06-19T13:23:51Z

@alyssawilk Thanks! I'll go with 1) now to get the PR unblocked, but I can change the PR to 3) in case two maintainers volunteer to own transport_sockets or transport_sockets/tls.

PiotrSikora · 2019-06-20T03:17:08Z

I'd be inclined to add @PiotrSikora, and I'm checking with @envoyproxy/maintainers to see if anyone else is willing to claim ownership

Works for me, thanks! :) (I'm assuming you meant transport_sockets/tls and not transport_sockets category as a whole, since I believe each transport socket should be treated as a separate extension).

alyssawilk · 2019-06-20T12:56:10Z

Absolutely. @ipuustin you can add @PiotrSikora and @lizan as owners of transport_sockets/tls which should take care of that and all subdirectories.

ipuustin · 2019-06-20T13:08:50Z

Ok, I added a patch to PR #6326 to do that. If the patch is needed before that PR lands then just go ahead and use it in a separate PR.

alyssawilk added 2 commits June 13, 2019 15:12

Adding owners check to format script

0ba535b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

Merge branch 'master' into owners

4e97fb9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk commented Jun 13, 2019

View reviewed changes

cleanup

f817c94

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the owners branch from a156b6a to f817c94 Compare June 13, 2019 19:26

mattklein123 requested changes Jun 13, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 13, 2019

mattklein123 self-assigned this Jun 13, 2019

dedup

7bb64ee

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jun 14, 2019

mattklein123 requested changes Jun 17, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Jun 17, 2019

alyssawilk added 2 commits June 17, 2019 13:07

Merge branch 'master' into owners

368c3c8

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

readability

5393bcd

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

repokitteh-read-only bot removed the waiting label Jun 17, 2019

mattklein123 requested changes Jun 17, 2019

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jun 17, 2019

mattklein123 added the waiting label Jun 17, 2019

format + comment

a47b652

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk force-pushed the owners branch from 40d4e67 to a47b652 Compare June 17, 2019 19:12

repokitteh-read-only bot removed the waiting label Jun 17, 2019

mattklein123 approved these changes Jun 17, 2019

View reviewed changes

mattklein123 merged commit 54e65f8 into envoyproxy:master Jun 17, 2019

ipuustin reviewed Jun 19, 2019

View reviewed changes

alyssawilk deleted the owners branch July 31, 2019 20:33

Conversation

alyssawilk commented Jun 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ipuustin commented Jun 19, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 19, 2019

Uh oh!

ipuustin commented Jun 19, 2019

Uh oh!

PiotrSikora commented Jun 20, 2019

Uh oh!

alyssawilk commented Jun 20, 2019

Uh oh!

ipuustin commented Jun 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants