Skip to content

tooling: Use upstream checkers#18087

Merged
htuch merged 4 commits intoenvoyproxy:mainfrom
phlax:tooling-upstream-pip-check
Sep 15, 2021
Merged

tooling: Use upstream checkers#18087
htuch merged 4 commits intoenvoyproxy:mainfrom
phlax:tooling-upstream-pip-check

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Sep 11, 2021

Commit Message: tooling: Use upstream pip check
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 11, 2021
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18087 was opened by phlax.

see: more, trace.

@phlax phlax marked this pull request as draft September 11, 2021 19:22
@phlax phlax force-pushed the tooling-upstream-pip-check branch from a15b9a1 to 599d874 Compare September 11, 2021 21:27
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-upstream-pip-check branch from 599d874 to 1d8ce2c Compare September 14, 2021 07:02
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the tooling-upstream-pip-check branch from 1d8ce2c to 7aeadc4 Compare September 14, 2021 07:05
@phlax phlax changed the title [WIP] tooling: Use upstream pip check [WIP] tooling: Use upstream checkers Sep 14, 2021
@phlax phlax changed the title [WIP] tooling: Use upstream checkers tooling: Use upstream pip check Sep 14, 2021
@phlax phlax changed the title tooling: Use upstream pip check tooling: Use upstream checkers Sep 14, 2021
@phlax phlax marked this pull request as ready for review September 14, 2021 07:07

def main(*args) -> int:
return PipChecker(*args).run()
return EnvoyPipChecker(*args).run()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these wrappers just for repository path magic? Is there a way to factor this out? I notice the path logic appears multiple times. Otherwise LGTM.

Copy link
Copy Markdown
Member Author

@phlax phlax Sep 15, 2021

Choose a reason for hiding this comment

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

i think we want this

if someone has some issue with eg the PipChecker they are going to look at this file and see that the only thing coming locally is the path, and that the rest of the logic is coming from the upstream package

with others it will not just be the path - but eg extension_categories or repository_locations etc etc

the object model here is designed to make these data sources explicit while shifting the actual implementation code upstream

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 15, 2021

/wait for docstring/comment updates

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax requested a review from htuch September 15, 2021 14:01
Signed-off-by: Ryan Northey <ryan@synca.io>

def _strip_lines(self, lines: Iterable[str]) -> List[str]:
return [self._strip_line(line) for line in lines if line]
def path(self) -> pathlib.Path:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As discussed offline, would be nice to get rid of this boilerplate. But this can be a distinct PR.

Copy link
Copy Markdown
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, thanks!

@htuch
Copy link
Copy Markdown
Member

htuch commented Sep 15, 2021

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 15, 2021
@htuch htuch merged commit 6fc65e2 into envoyproxy:main Sep 15, 2021
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.

2 participants