-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add google/gnostic
as a managed module
#549
Conversation
Tested: #550 |
+ /openapiv2/OpenAPIv2.proto | ||
|
||
# Open API v3 | ||
+ /openapiv3/annotations.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Present since release v0.6.8: #550 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I think we'd want to move them to proper folders based on the package names. For example, /openapiv2/OpenAPIv2.proto
defines package openapi.v2
so the file should be moved to openapi/v2/OpenAPIv2.proto
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not want all of these .proto files?
I did not add them because they were not requested in the original issue #482, just the openapiv2
and openapiv3
directories. cc @bufdev can you confirm if we also want those files pls?
I think we'd want to move them to proper folders based on the package names. For example,
/openapiv2/OpenAPIv2.proto
defines packageopenapi.v2
so the file should be moved toopenapi/v2/OpenAPIv2.proto
That would be a first, we usually respect the original directory structures for all other managed modules from source (which they usually match with the pkg name) I can move it to the correct directory, sure, but I think that would result in a mismatch for users looking to depend on them, import them, and realize the paths don't match with source, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the code that was requested for syncing, however, unless the module is untenably large I think we should default to including all relevant proto definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the relevant protos (excluding examples
and third_party
dirs), test-synced them, this is the most recent synced ref v0.7.0
: #551 (comment)
And about the movement of file to match their proto pkgs, all those other synced protos have pkg names that also don't match their parent dirs:
File path | Proto pkg |
---|---|
discovery/discovery.proto | package discovery.v1; |
extensions/extension.proto | package gnostic.extension.v1; |
metrics/lint/linter.proto | package linter; |
metrics/complexity.proto | package gnostic.metrics.v1; |
plugins/plugin.proto | package gnostic.plugin.v1; |
surface/surface.proto | package surface.v1; |
Moving all those files to their respective "expected dirs" according to the pkg names would result in a big change vs the original source paths. I'd say we should respect the dir structure from source?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The files in the original repo are in the proper locations: https://buf.build/gnostic/gnostic/tree/main:gnostic/openapi/v3
I think we should match if this is intended to replace that repo.
This reverts commit b8fb9a8.
Looking at this thread, we are turning this PR into a draft while that protos restructuring happen, so we don't deviate from the original intended paths/pkgs names from the author. For now, in the public BSR, interested users can depend on the module that the author is already maintaining here: https://buf.build/gnostic/gnostic/docs/main:gnostic.openapi.v3, and for Pro and Enterprise customers we can recommend |
Closing it for now. We can reopen/reevaluate later. |
Fixes #482