Skip to content

repo reorg: move network/HTTP ext_authz filters#2915

Closed
mattklein123 wants to merge 4 commits intomasterfrom
move_ext_auth
Closed

repo reorg: move network/HTTP ext_authz filters#2915
mattklein123 wants to merge 4 commits intomasterfrom
move_ext_auth

Conversation

@mattklein123
Copy link
Member

Demonstrates common code across multiple filters.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Demonstrates common code across multiple filters.

Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

Oops need to kill some Instance. One sec.

Signed-off-by: Matt Klein <mklein@lyft.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one question..

namespace Envoy {
namespace Extensions {
namespace Filters {
namespace Common {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works well for ext_authz, +1. I'm wondering how you think this structure plays out when we have two HTTP filters with common code (rather than a network and an HTTP file sharing some common code)? Do we still use filters/common, or is there filters/http/common?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning on doing filters/http/common and I would do Extensions::HttpFilters::Common::.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would find it really helpful if we had it documented somewhere what goes where and why, as part of this series of PRs. Internally we have a requirements on BUILD files they have a top level comment on what goes where but many of our intermediate directories don't have BUILD files (yet). I'm open to BUILD files in the "empty" directories, or FOO.md files, or a REPO_ORGANIZATION file in a top level directory, or comments on each module why they live where they live, whatever works!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to go ahead and add a REPO_ORGANIZATION.md to the top level in this PR. I think that's a great idea.

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
@mattklein123
Copy link
Member Author

Closing temporarily until I get the doc in place.

@mattklein123
Copy link
Member Author

Oops I guess I can't reopen this. Will open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants