Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] tooling: Add jsonschema dump of API #13283

Closed
wants to merge 8 commits into from

Conversation

phlax
Copy link
Member

@phlax phlax commented Sep 26, 2020

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

Commit Message: tooling: Add jsonschema dump of API
Additional Description:
Add jsonschema dump of API that can be consumed in tooling such as VSCode

Risk Level: low
Testing:
Docs Changes: to follow
Release Notes:
[Optional Runtime guard:]
[Optional Fixes #Issue] Fix #13254
[Optional Deprecated:]

@phlax phlax marked this pull request as draft September 26, 2020 09:18
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-watchers: FYI only for changes made to (bazel/repository_locations\.bzl)|(api/bazel/repository_locations\.bzl)|(.*/requirements\.txt).

🐱

Caused by: #13283 was synchronize by phlax.

see: more, trace.

@phlax phlax force-pushed the api-dump-to-jsonschema branch 2 times, most recently from a83a3fc to da2bbcb Compare September 27, 2020 18:11
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the api-dump-to-jsonschema branch from c43a7bf to 8e0b6dc Compare September 28, 2020 12:16
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>
@@ -10,3 +11,10 @@ go_proto_compiler(
valid_archive = False,
visibility = ["//visibility:public"],
)

go_binary(
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if this is correct/necessary - i figured we need to compile the lib/bin somewhere - locally i did it with make build and then exporting ./bin to PATH

"//tools/api_proto_plugin",
"//tools/config_validation:validate_fragment",
"@com_envoyproxy_protoc_gen_validate//validate:validate_py",
"@com_github_chrusty_protoc_gen_jsonschema",
Copy link
Member Author

Choose a reason for hiding this comment

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

adding this fails - im not sure how to include the lib so its visible to api_proto_plugin_impl

Copy link
Member Author

Choose a reason for hiding this comment

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

seems like one problem im hitting is because protoc_gen_jsonschema doesnt have a bazel BUILD file

im wondering if i need to "inject" a BUILD file as suggested here https://grpc.io/blog/bazel-rules-protobuf/#143-workspace-rules-that-accept-a-build-file-as-an-argument, or whether there is an easier way to include the compiled binary

also, not sure if/how its possible to use rules_proto - i had a look at it - but im struggling to figure out how it can be used with protoc-gen-jsonschema

Copy link
Member

Choose a reason for hiding this comment

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

A few things:

  1. You probably do need to inject a BUILD file (or upstream one) to turn protoc-gen-jsonschema into a go_binary.
  2. I think the interesting thing from api_proto_plugin isn't the Python framework, which doesn't really apply here, but the .bzl, i.e. tools/api_proto_plugin/plugin.bzl, which gives essentially a way to invoke some protoc plugin as an aspect.
  3. For rules_proto, I think you want to figure out how to use proto_plugin based on how some of the language examples work in the repo, e.g. cpp_proto_compile. If we go this route, we could either do an aspect like in (2), or modify api_proto_library to also externalize a jsonscheme target for each proto library.

Copy link
Member Author

Choose a reason for hiding this comment

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

im gonna have to get a better handle on how these components work independently of envoy repo - ill update once i have a vague clue what im doing...

@phlax
Copy link
Member Author

phlax commented Sep 28, 2020

@htuch i copied out the docs build (+ protodoc), and then gradually paired it down to things that seemed to be necessary.

im not sure if i have removed stuff that is required - and almost certainly there is cruft left that still needs to be removed.

im not quite sure how to procede from here - ive left a couple of comments inline - basically it downloads protoc-gen-jsonschema - not sure if its compiling it, or how to include it into the bazel build from here

@phlax phlax changed the title [WIP] tooling: Add jsonschema dump of API [WIP/RFC] tooling: Add jsonschema dump of API Sep 28, 2020
if [ -n "$CIRCLE_TAG" ]
then
# Check the git tag matches the version number in the VERSION file.
VERSION_NUMBER=$(cat VERSION)
Copy link
Member

Choose a reason for hiding this comment

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

Can you refactor with docs/build.sh to a util shell script?

Copy link
Member Author

Choose a reason for hiding this comment

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

iiuc - yep

echo "Generating ${API_VERSION} API SCHEMA..."

# Generate the extensions docs
echo "RUNNING BAZEL BUILD WITH ASPECT..."
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Prefer not-all caps, too much shouting :)

Copy link
Member Author

Choose a reason for hiding this comment

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

SORRY, DEBUGGING 8/

EXTENSION_DB_PATH="$(realpath "${BUILD_DIR}/extension_db.json")"
export EXTENSION_DB_PATH

# This is for local RBE setup, should be no-op for builds without RBE setting in bazelrc files.
Copy link
Member

Choose a reason for hiding this comment

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

Same here for refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

yep

"//tools/api_proto_plugin",
"//tools/config_validation:validate_fragment",
"@com_envoyproxy_protoc_gen_validate//validate:validate_py",
"@com_github_chrusty_protoc_gen_jsonschema",
Copy link
Member

Choose a reason for hiding this comment

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

A few things:

  1. You probably do need to inject a BUILD file (or upstream one) to turn protoc-gen-jsonschema into a go_binary.
  2. I think the interesting thing from api_proto_plugin isn't the Python framework, which doesn't really apply here, but the .bzl, i.e. tools/api_proto_plugin/plugin.bzl, which gives essentially a way to invoke some protoc plugin as an aspect.
  3. For rules_proto, I think you want to figure out how to use proto_plugin based on how some of the language examples work in the repo, e.g. cpp_proto_compile. If we go this route, we could either do an aspect like in (2), or modify api_proto_library to also externalize a jsonscheme target for each proto library.

@phlax phlax changed the title [WIP/RFC] tooling: Add jsonschema dump of API [WIP] tooling: Add jsonschema dump of API Sep 30, 2020
Signed-off-by: Ryan Northey <ryan@synca.io>
@stale
Copy link

stale bot commented Oct 12, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 12, 2020
@phlax
Copy link
Member Author

phlax commented Oct 12, 2020

bump, im working on this

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 12, 2020
This was referenced Oct 21, 2020
Signed-off-by: Ryan Northey <ryan@synca.io>
@repokitteh-read-only
Copy link

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: #13283 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 Nov 9, 2020
@phlax
Copy link
Member Author

phlax commented Nov 9, 2020

with a lot of hackery i have kinda got this working locally

i havent pushed the code yet, as its pretty messy, and doesnt work exactly

to get it working i forked protoc-gen-jsonschema and added BUILD files etc, altho i reckon i could have avoided that in retrospect

protoc-gen-jsonschema builds correctly and has all its deps afaict - and is being called in the protoschema build

the remaining problem seems to be that some of the proto files have multiple messages or enums

p-g-j handles this by creating separate jsonschema files for each. Seems like bazel doesnt like that.

@phlax
Copy link
Member Author

phlax commented Nov 9, 2020

after a bit more experimenting with this p-g-j creates files using the message name so eg if there is a file foo.proto containing a Foo message it creates a file Foo.proto.jsonschema

this allows for multiple messages or enums

i need to clarify how jsonschema works in this respect - maybe we need to have multiple files for this.

Atm bazel fails with this tho as it expects different outputs

to get it ~working i hacked p-g-j and made it output to the expected file and skip any additional messages. It kinda works, other things fail, but most of the proto files get jsonschema files - not sure of their veracity at this stage

@htuch
Copy link
Member

htuch commented Nov 10, 2020

@phlax Bazel's build graph needs to have known output files at each node. One technique that is sometimes used is to take multiple unknown output files and combine them into a single tar for the node.

@phlax
Copy link
Member Author

phlax commented Nov 10, 2020

re protoc-gen-jsonschema i think its pretty likely that it wont just work out of the box - at very least im not sure that it will handle typed_config in the way we want it to

i guess the main question im trying to answer atm is whether we can wrap and mangle the output from p-g-j (and potentially upstream any small changes that we need) - or whether our needs are quite specific and we are better off with a forked/hacked version

ill keep playing with this and post updates

@github-actions
Copy link

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 Dec 10, 2020
@github-actions
Copy link

This pull request has been automatically closed because it has not had activity in the last 37 days. 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 closed this Dec 17, 2020
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 stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add jsonschema dump of API protocols
2 participants