Skip to content

Sync rekor-tiles protos into protobuf-specs#661

Merged
loosebazooka merged 5 commits intomainfrom
sync-rekor-tiles
Jun 6, 2025
Merged

Sync rekor-tiles protos into protobuf-specs#661
loosebazooka merged 5 commits intomainfrom
sync-rekor-tiles

Conversation

@loosebazooka
Copy link
Copy Markdown
Member

@loosebazooka loosebazooka commented Jun 5, 2025

  • sync is by manually running service-protos/sync-rekor-tiles.sh
    • this current commit does HEAD
  • pull in latest tag (it doesn't do releases) from rekor-tiles
  • rewrite the go_package to match protobuf specs
  • do NOT put service protos in ./protos because we export those in the service-builder container image
  • rework language builder to include ./service-protos
  • include ./service-protos in exported jar
  • remove jsonschema
  • rust updated (thanks @jku)

Summary

Release Note

Documentation

@loosebazooka
Copy link
Copy Markdown
Member Author

loosebazooka commented Jun 5, 2025

Looks like typescript would need an import of "bufbuild/wire", this is only a problem because we're also sync'ing in the service definition. I can "exclude" that from the sync. service-protos kind of takes on a different meaning then

@jku
Copy link
Copy Markdown
Member

jku commented Jun 5, 2025

handle rust code gen in the rust codebase?

if the issue is that you didn't want to poke the custom rust codegen code, I can give it a try (no promises on results though)

@loosebazooka
Copy link
Copy Markdown
Member Author

if the issue is that you didn't want to poke the custom rust codegen code, I can give it a try (no promises on results though)

I took a quick look, and decided someone else maybe would want to. I don't think it's very complicated, was just worried I was gonna burn a bunch of hours

@jku
Copy link
Copy Markdown
Member

jku commented Jun 5, 2025

Two commits for rust in https://github.com/jku/protobuf-specs/commits/sync-rekor-tiles/ if you want them (first is the codegen change, second is "make rust" result)

(I can make a separate PR as well, let me know)

- sync is by manually running service-protos/sync-rekor-tiles.sh
- pull in latest tag (it doesn't do releases) from rekor-tiles
- rewrite the go_package to match protobuf specs
- ignore protos for grpc service defintions
- do NOT put service protos in ./protos because we export those
  in the service-builder container image
- rework language builder to include ./service-protos
- include ./service-protos in exported jar
- remove jsonschema

TODO:
- handle rust code gen in the rust codebase?

Signed-off-by: Appu Goundan <appu@google.com>
@loosebazooka
Copy link
Copy Markdown
Member Author

oh woops, I updated, amended and force pushed. I'll cherry pick in your diffs

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@loosebazooka loosebazooka marked this pull request as ready for review June 5, 2025 18:04
Hayden-IO
Hayden-IO previously approved these changes Jun 5, 2025
Copy link
Copy Markdown
Collaborator

@Hayden-IO Hayden-IO left a comment

Choose a reason for hiding this comment

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

Great work on this!

Do you want to add a nightly GHA that tracks when there are new tags and changes to the protos in all services?

Makefile Outdated
${DOCKER_RUN} -v ${PWD}:/defs ${PROTOC_RUBY_IMAGE} ${PROTO_INCLUDES}\
--ruby_out=/defs/gen/pb-ruby/lib ${PROTOS}

# TODO: serivce-proto code gen for rust
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Delete TODO?

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.

doh

@loosebazooka
Copy link
Copy Markdown
Member Author

Do you want to add a nightly GHA that tracks when there are new tags and changes to the protos in all services?

I can add it in a followup, this is already kind of a hefty PR.

Signed-off-by: Appu Goundan <appu@google.com>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be here, or is this leftover from a generation with the service proto?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe try a make clean; make all to confirm?

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.

I thought I did, weird.

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.

gotta update the makefile

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.

I think there are some other ruby things in there that might need updating: https://github.com/sigstore/protobuf-specs/blob/main/gen/pb-ruby/lib/sigstore_protobuf_specs.rb

@segiddins ?

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.

we're not using the generated protos from this repo since they depend on the protobuf gem

Signed-off-by: Appu Goundan <appu@google.com>
@loosebazooka
Copy link
Copy Markdown
Member Author

loosebazooka commented Jun 5, 2025

it looks like typescript needs an update too, to: https://github.com/sigstore/protobuf-specs/blob/main/gen/pb-typescript/src/index.ts @bdehamer ?

Hayden-IO
Hayden-IO previously approved these changes Jun 5, 2025
@loosebazooka
Copy link
Copy Markdown
Member Author

Should I add TODOs/issues for typescript and ruby?

@Hayden-IO
Copy link
Copy Markdown
Collaborator

What's the current issue(s)? Given we aren't immediately targeting JS/Ruby Rekor v2 support, filing issues for now would be fine. Should we omit js/ruby entirely if there are issues?

Copy link
Copy Markdown
Member

@jku jku left a comment

Choose a reason for hiding this comment

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

leaving further ts and ruby changes to issues seems fine, let's just ping the relevant folks

Comment on lines +20 to +27
git clone --filter=blob:none --no-checkout --depth=1 https://github.com/sigstore/rekor-tiles.git ./rekor-tiles && \
cd ./rekor-tiles && \
git sparse-checkout set --no-cone '/api/proto/rekor/v2/*.proto' '!**/rekor_service.proto' && \
git fetch origin tag "$latest_tag" --no-tags && \
git checkout "$latest_tag" && \
cd ../ && \
cp -R ./rekor-tiles/api/proto/* . && \
rm -rf ./rekor-tiles
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.

was there a reason to use "&&" here? You use errexit already so it shouldn't be needed

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.

No. Started with a script from somewhere else and continued with that style. It's unnecessary, I wasnt thinking.

@jku
Copy link
Copy Markdown
Member

jku commented Jun 6, 2025

This is just a note, not suggesting any changes:

do NOT put service protos in ./protos

I thought this would have no affect on the sigstore-python PR but it turns out we did actually use CreateEntryRequest from the service protos to build the request message. Doing that manually is not an issue.

@loosebazooka
Copy link
Copy Markdown
Member Author

I thought this would have no affect on the sigstore-python PR but it turns out we did actually use CreateEntryRequest from the service protos to build the request message. Doing that manually is not an issue.

Oh I didn't think about that. Hrmmm.. seems silly to not include it.

Signed-off-by: Appu Goundan <appu@google.com>
@loosebazooka
Copy link
Copy Markdown
Member Author

okay cleaned up the script a bit

@loosebazooka
Copy link
Copy Markdown
Member Author

oh shoot, jku contributed to this PR so need another review.

Copy link
Copy Markdown
Collaborator

@Hayden-IO Hayden-IO left a comment

Choose a reason for hiding this comment

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

@loosebazooka does this need to be rerun with the new tag or is this generated from a fix locally?

@loosebazooka
Copy link
Copy Markdown
Member Author

Will merge, and rerun with updates on a new tag

@loosebazooka loosebazooka merged commit b7e49be into main Jun 6, 2025
27 checks passed
@loosebazooka loosebazooka deleted the sync-rekor-tiles branch June 6, 2025 15:50
@bdehamer
Copy link
Copy Markdown
Collaborator

bdehamer commented Jun 9, 2025

Open #669 to expose these new types as part of the generated npm package.

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.

5 participants