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

Add google/gnostic as a managed module #549

Closed
wants to merge 4 commits into from

Conversation

unmultimedio
Copy link
Member

Fixes #482

@unmultimedio
Copy link
Member Author

Tested: #550

+ /openapiv2/OpenAPIv2.proto

# Open API v3
+ /openapiv3/annotations.proto
Copy link
Member Author

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)

Copy link
Member

Choose a reason for hiding this comment

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

image

Do we not want all of these .proto files?

Copy link
Member

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?

Copy link
Member Author

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 package openapi.v2 so the file should be moved to openapi/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?

Copy link
Member

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.

Copy link
Member Author

@unmultimedio unmultimedio May 14, 2024

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?

Copy link
Member

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.

@unmultimedio
Copy link
Member Author

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 buf export and buf push in the meantime.

@unmultimedio unmultimedio marked this pull request as draft May 14, 2024 19:00
@unmultimedio
Copy link
Member Author

Closing it for now. We can reopen/reevaluate later.

@unmultimedio unmultimedio deleted the jfigueroa/google-gnostic branch July 2, 2024 20:18
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.

Add managed module: buf.build/google/gnostic
3 participants