Skip to content

deps: Add pip check tool#15802

Merged
htuch merged 7 commits intoenvoyproxy:mainfrom
phlax:deps-add-pip-check
Apr 2, 2021
Merged

deps: Add pip check tool#15802
htuch merged 7 commits intoenvoyproxy:mainfrom
phlax:deps-add-pip-check

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Apr 1, 2021

Signed-off-by: Ryan Northey ryan@synca.io

Commit Message: deps: Add pip check tool
Additional Description:

adds a tool to check that all pip requirements are properly specified for dependabot

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] Fix: #15665
[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 Apr 1, 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: #15802 was opened by phlax.

see: more, trace.

@phlax phlax changed the title [WP] deps: Add pip check tool [WIP] deps: Add pip check tool Apr 1, 2021
@phlax phlax marked this pull request as draft April 1, 2021 08:53
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Apr 1, 2021

i may add some additional functionality - specifically im thinking it could create venvs and do pip install -r requirements.txt for each of the requirements

this would pick up bugs like the recent ones where dependabot has updated a pip requirement but not all requirements were triggered in the relevant pr

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15802 was synchronize by phlax.

see: more, trace.

@phlax phlax force-pushed the deps-add-pip-check branch from c74a9df to bdea782 Compare April 1, 2021 09:01
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Apr 1, 2021

apologies for bad rebase 8/

@phlax phlax removed the api label Apr 1, 2021
@phlax phlax force-pushed the deps-add-pip-check branch 2 times, most recently from cfb9d5e to 3b06efa Compare April 1, 2021 10:40
@phlax phlax force-pushed the deps-add-pip-check branch 2 times, most recently from 4d4c97e to 156ef97 Compare April 1, 2021 10:50
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the deps-add-pip-check branch from 156ef97 to 925c2b5 Compare April 1, 2021 10:55
phlax added 4 commits April 1, 2021 12:05
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
phlax added 2 commits April 1, 2021 13:16
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax changed the title [WIP] deps: Add pip check tool deps: Add pip check tool Apr 1, 2021
@phlax phlax marked this pull request as ready for review April 1, 2021 12:20
@phlax phlax requested review from htuch and moderation April 1, 2021 12:21
return set(
root[len(self.path):]
for root, dirs, files in os.walk(self.path)
if "requirements.txt" in files)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

use a constant and add a note about hardcoding requirements.txt



# TODO(phlax): move this to a base module
class Checker(object):
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.

@phlax can this verify that we're doing pinning for all requirements.txt? CC @moderation

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

not yet, but yes, kinda in 2 ways

we could add pip-compile which is kinda like pipfmt, and can check/generate hashes

secondly we could/should run a bazel query to ensure that all of the pip targets use require-hashes

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

can we do this in a follow up, it brings quite a few considerations beyond this 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 Apr 2, 2021

/lgtm deps

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

Add some CI to ensure that dependabot.yml is up to date

3 participants