Skip to content

[WIP] bazel: Add python data providers#17998

Closed
phlax wants to merge 1 commit intoenvoyproxy:mainfrom
phlax:bazel-data
Closed

[WIP] bazel: Add python data providers#17998
phlax wants to merge 1 commit intoenvoyproxy:mainfrom
phlax:bazel-data

Conversation

@phlax
Copy link
Copy Markdown
Member

@phlax phlax commented Sep 6, 2021

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

Commit Message: bazel: Add python data providers
Additional Description:

This is a generalized solution to the problem of getting data from bazel into python tools

It adds a write_json rule that can create a json target from any data serializable by the bazel json parser

It also adds a py_data rule which allows json or yaml to be imported directly into python

The data rule take an optional filters argument which is a chain of mutating rules. The filters deal with python data and are agnostic to the source, but are obv specific to the data being processsed.

This PR adds the required macros and adds rules for ~all of the data currently used in tooling

It doesnt make use of the data - this will be added in a follow up PR

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@phlax phlax changed the title bazel: Add json/yaml writers and python providers [WIP] bazel: Add json/yaml writers and python providers Sep 6, 2021
@phlax phlax marked this pull request as draft September 6, 2021 09:31
@phlax phlax force-pushed the bazel-data branch 5 times, most recently from c4cbb99 to 93d84b9 Compare September 6, 2021 12:01
@phlax phlax changed the title [WIP] bazel: Add json/yaml writers and python providers bazel: Add json/yaml writers and python providers Sep 6, 2021
@phlax phlax marked this pull request as ready for review September 6, 2021 12:21
@phlax phlax requested review from htuch and mattklein123 September 6, 2021 12:22
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 6, 2021

cc @mattklein123 @htuch

although this adds some code, it will allow us to reduce code overall, and exposes bazel and bazel data in ways that fits both with bazel's workflow and python's idioms, i think

ultimately, it will be very useful shifting any remaining scripts to bazel and tidying up some of the existing bazel rules

@phlax phlax force-pushed the bazel-data branch 2 times, most recently from 9a35c54 to 391b659 Compare September 6, 2021 12:45
Copy link
Copy Markdown
Member Author

@phlax phlax Sep 6, 2021

Choose a reason for hiding this comment

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

cc @envoyproxy/dependency-shepherds

this rule doesnt make sense in the api, which is mounted within an external namespace in envoy' workspace, and can only refer to the envoy targets through the @envoy namespace

perhaps the intention is to prevent accessing the envoy namespace from the api, but then that feels more like a job for visibility

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.

this could be resolved alternately by shifting these rules into a separate namespace

without this change, the current implementation fails format checking - i guess for good reason

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 6, 2021

the one question im wondering about this is whether it would be better to have the mutators in the write_json and/or as implemented in the python accessors

the benefit to having the mutators in the write method would be

  • non-python rules can make use of the mutated data
  • if mutation is expensive then it would benefit from bazel caching etc

the downside is added code/complexity - its easier to implement as it is, as there is already a need to add a python template and inject it with data

@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 6, 2021

the other thing is namespacing

with current implementation you can import "from" the repo - ie from source.extensions import extensions_build_config

with #17293 the envoy_repo namespace is introduced - which could allow something like from envoy_repo.source.extensions import ...

potentially, we could create a directory like tools/data (or just data) and mount that into the the envoy_repo namespace

rules would have to depend on @envoy_repo data in that case - which i guess is also more explicit from a bazel pov

@phlax phlax force-pushed the bazel-data branch 2 times, most recently from 54108d4 to be897af Compare September 6, 2021 13:39
@phlax phlax changed the title bazel: Add json/yaml writers and python providers bazel: Add json writers and json/yaml python providers Sep 6, 2021
@phlax phlax changed the title bazel: Add json writers and json/yaml python providers bazel: Add json writer and json/yaml python providers Sep 6, 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: #17998 was synchronize by phlax.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 6, 2021
@phlax phlax force-pushed the bazel-data branch 3 times, most recently from 32084ff to 2ae7915 Compare September 16, 2021 07:38
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Sep 17, 2021

@lizan @htuch before pushing this PR i would like to go through all of the ways in which i want to use it and prepare the following PRs, mostly so i can think through the pattern

i would v much like this or something like this to land, and believe i can cut quite a bit of code with it, but i would like to think it through again, so its going to take me a few days

@phlax phlax force-pushed the bazel-data branch 11 times, most recently from eb708de to 34a22a3 Compare September 20, 2021 20:43
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.

Tuple[str, ...]

@phlax phlax force-pushed the bazel-data branch 2 times, most recently from 218a3d8 to 219f477 Compare September 21, 2021 09:17
Signed-off-by: Ryan Northey <ryan@synca.io>
@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 21, 2021
@phlax phlax added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Oct 21, 2021
htuch pushed a commit that referenced this pull request Jan 11, 2022
There are a few tools that require the repository_locations data, currently we are bringing that data into the python by putting the implementation (as opposed to the abstract library) in the envoy repo, this will allow us to instead pass the repo locations data as a file/cli flag, and move the implementation into the upstream tooling

This PR also avoids using the old SourceFileLoader + bzl files method of loading the data, instead loading directly from the write_json rules for the repo files and interpolating

There is a more generic implementation of some of what is required here in #17998
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax
Copy link
Copy Markdown
Member Author

phlax commented Jan 20, 2022

with the move to pytooling this should not be necessary, we can use json data from bazel

@phlax phlax closed this Jan 20, 2022
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
There are a few tools that require the repository_locations data, currently we are bringing that data into the python by putting the implementation (as opposed to the abstract library) in the envoy repo, this will allow us to instead pass the repo locations data as a file/cli flag, and move the implementation into the upstream tooling

This PR also avoids using the old SourceFileLoader + bzl files method of loading the data, instead loading directly from the write_json rules for the repo files and interpolating

There is a more generic implementation of some of what is required here in envoyproxy#17998
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Josh Perry <josh.perry@mx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deps Approval required for changes to Envoy's external dependencies no stalebot Disables stalebot from closing an issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants