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

Added a prototype for generating jsonschema files #112

Merged
merged 10 commits into from
Jul 31, 2023

Conversation

kommendorkapten
Copy link
Member

WIP to generate JSON schema files, as coined by @woodruffw (see Slack: https://sigstore.slack.com/archives/C03SZ6SHU90/p1690490585606889)

Closes #67

Consider this PR WIP so far.

There are some general topics I would like to bring up for discussion too related to the current Docker image we are using for building the protoc compiler that I discovered when working on this. I'll create a separate issue for that.

Release Note

Added support for generating JSON schema files

Documentation

N/A

@kommendorkapten
Copy link
Member Author

The reason I added a new Docker image is that namely/protoc-all is too old to build newer Go programs, see issue #113

woodruffw
woodruffw previously approved these changes Jul 28, 2023
Copy link
Member

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I did a quick pass over the generated schemas and nothing looks way off 🙂

I've asked @jleightcap to take a look at using these to generate the Rust crate, once he's finished with some conformance suite work.

Makefile Outdated Show resolved Hide resolved
@kommendorkapten
Copy link
Member Author

kommendorkapten commented Jul 31, 2023 via email

@woodruffw
Copy link
Member

If any of you are in a hurry to get the JSON schema in now, feel free to modify this PR. Maintainers will have permission to do so.

I'm going to have @jleightcap work off of the schema in this PR, so I'll take over! Enjoy your vacation!

Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
# This is required to get the field_behavior.proto file
# NOTE: --filter=tree:0 performs a treeless clone; we do this to optimize cloning
# this otherwise relatively heavy repository.
RUN git clone --filter=tree:0 https://github.com/googleapis/googleapis.git
Copy link
Member

Choose a reason for hiding this comment

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

NB: This repository doesn't appear to be versioned in any way, and isn't packaged in alpine. I could check out a fixed revision if we're like more consistency here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to pin it to a fixed revision.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is moderately annoying: we can't do a fast-path clone with a specific revision. I'll see about just downloading it directly as a ZIP or similar and unarchiving.

Copy link
Member

Choose a reason for hiding this comment

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

Did it with a checkout after all; the performance shouldn't be too bad.

@@ -0,0 +1,8 @@
# 3.18.2
FROM alpine@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70
RUN apk add --update protoc=3.21.12-r2 protobuf-dev=3.21.12-r2 go=1.20.5-r0 git=2.40.1-r0
Copy link
Member

Choose a reason for hiding this comment

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

NB: I picked these version numbers not because they're the most recent, but because they're consistent with Alpine's world version for the Alpine version chosen. More recent versions could probably be made to work, but I figured this was the path of least resistance.

znewman01
znewman01 previously approved these changes Jul 31, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

This LGTM, except I would like the googleapis/googleapis repo pinned too :)

# This is required to get the field_behavior.proto file
# NOTE: --filter=tree:0 performs a treeless clone; we do this to optimize cloning
# this otherwise relatively heavy repository.
RUN git clone --filter=tree:0 https://github.com/googleapis/googleapis.git
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to pin it to a fixed revision.

# 3.18.2
FROM alpine@sha256:25fad2a32ad1f6f510e528448ae1ec69a28ef81916a004d3629874104f8a7f70
RUN apk add --update protoc=3.21.12-r2 protobuf-dev=3.21.12-r2 go=1.20.5-r0 git=2.40.1-r0
RUN go install github.com/chrusty/protoc-gen-jsonschema/cmd/[email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know if dependabot will suggest updates for Go dependencies in Dockerfiles? Otherwise we might want to add a hack like https://github.com/sigstore/rekor/blob/main/hack/tools/go.mod

znewman01
znewman01 previously approved these changes Jul 31, 2023
@haydentherapper
Copy link
Collaborator

Should the jsonschema be versioned and have releases cut along with the other languages? Otherwise, it'll always be from HEAD. We could have folders for each version, /schema/v0.2/? Not sure if there's precedent for this.

@woodruffw
Copy link
Member

Should the jsonschema be versioned and have releases cut along with the other languages? Otherwise, it'll always be from HEAD. We could have folders for each version, /schema/v0.2/? Not sure if there's precedent for this.

Hmm, good call -- we want to use this as the building block for the Rust bindings, so we should probably version it. IMO that can be done either by tagging (like the other bindings) or by having directories for either version; I don't think it'll make a major difference to the Rust bindings either way.

@haydentherapper
Copy link
Collaborator

Is it sufficient to use an existing tag like https://github.com/sigstore/protobuf-specs/releases/tag/v0.2.0? Otherwise we could have its own tag, releases/jsonschema/vXYZ, but I can't think of a case where we'll have the json schema version deviate.

@znewman01
Copy link
Contributor

Is it sufficient to use an existing tag like https://github.com/sigstore/protobuf-specs/releases/tag/v0.2.0? Otherwise we could have its own tag, releases/jsonschema/vXYZ, but I can't think of a case where we'll have the json schema version deviate.

In the past, we liked versioning each set of bindings independently to handle changes to the generation process. E.g. if there was a bug in the protoc-jsonschema generator. I think we should default to that if possible.

@haydentherapper
Copy link
Collaborator

SG, so we can just add a step to the release instructions to add a tag for release/json/vX.Y.Z, and we shouldn't need another release workflow.

@woodruffw
Copy link
Member

SGTM! I'll add a note to the release instructions.

@woodruffw woodruffw merged commit 9abc8c2 into sigstore:main Jul 31, 2023
@kommendorkapten kommendorkapten deleted the jsonschema-wip branch August 14, 2023 06:14
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.

Generate JSON Schema from Protobuf definitions and use it to enforce fields?
5 participants