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

impl: devtools/cmd/generate,testdata: add command to run protoc-gen #72

Merged
merged 2 commits into from
Nov 3, 2024

Conversation

julieqiu
Copy link
Member

@julieqiu julieqiu commented Nov 2, 2024

No description provided.

@julieqiu julieqiu requested review from codyoss and coryan November 2, 2024 18:23
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I think we should not merge the files that we are not going to use (**/BUILD.bazel and **_grpc_service_config.json. Maybe there is a good reason to merge them?

We also seem to have 3 different ways to generate the golden files now: this, my changes in #66, and the explicit call to protoc.

generator/testdata/google/cloud/secretmanager/BUILD.bazel Outdated Show resolved Hide resolved
@@ -0,0 +1,31 @@
// Copyright 2015 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

There is the metaphysical discussion around what is the copyright year for a copy of a file. LOL.

Alternative: we could use a git submodule and reference github.com/googleapis/googleapis instead of copying a few files

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered this option but I thought that it might be nice to just limit to the set of files we need so the input is clear. Happy to change this later on though

generator/README.md Show resolved Hide resolved
@julieqiu
Copy link
Member Author

julieqiu commented Nov 3, 2024

I think we should not merge the files that we are not going to use (**/BUILD.bazel and **_grpc_service_config.json. Maybe there is a good reason to merge them?

We also seem to have 3 different ways to generate the golden files now: this, my changes in #66, and the explicit call to protoc.

What do you think about moving everything under testdata/

We could have the structure:

testdata/
    googleapis/...
    openapi/secretmanager_openapi_v1.json
    golden/ 
       rust/
       go/

I can move this in a follow up PR if that seems good to you.

@coryan
Copy link
Contributor

coryan commented Nov 3, 2024

I think we should not merge the files that we are not going to use (**/BUILD.bazel and **_grpc_service_config.json. Maybe there is a good reason to merge them?
We also seem to have 3 different ways to generate the golden files now: this, my changes in #66, and the explicit call to protoc.

What do you think about moving everything under testdata/

We could have the structure:

testdata/
    googleapis/...
    openapi/secretmanager_openapi_v1.json
    golden/ 
       rust/
       go/

I can move this in a follow up PR if that seems good to you.

A single location SGTM. In case you missed this: we will need more than one golden/rust/ and golden/go/ directories because we will generate files with conflicting names from OpenAPI and from Protobuf.

@julieqiu julieqiu merged commit 382af5f into main Nov 3, 2024
15 checks passed
@julieqiu julieqiu deleted the julie-pr5 branch November 3, 2024 16:53
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.

2 participants