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

Migrate gnostic to use gnostic-models #400

Merged
merged 3 commits into from
Aug 25, 2023
Merged

Migrate gnostic to use gnostic-models #400

merged 3 commits into from
Aug 25, 2023

Conversation

pkwarren
Copy link
Contributor

In order to avoid a panic at runtime in a downstream project that pulls in gnostic and gnostic-models, create a new <proto>.pbalias.go file which uses type aliases and variables to point to the equivalent protobuf types in gnostic-models. This will prevent any API breakage for code using the previous types from gnostic, and allow for this logic to be maintained in one place going forward.

Fixes #397.

In order to avoid a panic at runtime in a downstream project that pulls
in gnostic and gnostic-models, create a new `<proto>.pbalias.go` file
which uses type aliases and variables to point to the equivalent
protobuf types in gnostic-models. This will prevent any API breakage for
code using the previous types from gnostic, and allow for this logic to
be maintained in one place going forward.
protoc -I . -I ./third_party --go_out=. --go_opt=paths=source_relative surface/*.proto
protoc -I . -I ./third_party --go_out=. --go_opt=paths=source_relative metrics/*.proto
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the directories which have proto definitions in gnostic-models, and noticed metrics was compiling twice.

go.mod Outdated Show resolved Hide resolved
@timburks
Copy link
Contributor

timburks commented Aug 4, 2023

@pkwarren is there an update needed now that google/gnostic-models#5 is merged?

@pkwarren
Copy link
Contributor Author

pkwarren commented Aug 4, 2023

@pkwarren is there an update needed now that google/gnostic-models#5 is merged?

Yes - thanks for merging the dependency. I was out today but will aim to get this ready tomorrow.

@pkwarren
Copy link
Contributor Author

@timburks - This should be ready for re-review now. Let me know if you have any questions.

@timburks
Copy link
Contributor

Thanks @pkwarren LGTM - I'm not sure of all the downstream implications but I'm game to try this (and clients should be using tagged versions). Merging.

@timburks timburks merged commit 836f55b into google:main Aug 25, 2023
1 check passed
@pkwarren pkwarren deleted the pkw/issue-397 branch August 25, 2023 19:48
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.

Import of gnostic-models and gnostic generated code leads to panic
2 participants