Skip to content

Migrate gnostic to use gnostic-models#400

Merged
timburks merged 3 commits into
google:mainfrom
pkwarren:pkw/issue-397
Aug 25, 2023
Merged

Migrate gnostic to use gnostic-models#400
timburks merged 3 commits into
google:mainfrom
pkwarren:pkw/issue-397

Conversation

@pkwarren

Copy link
Copy Markdown
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.
Comment thread COMPILE-PROTOS.sh
protoc -I . -I ./third_party --go_out=. --go_opt=paths=source_relative plugins/*.proto
protoc -I . -I ./third_party --go_out=. --go_opt=paths=source_relative extensions/*.proto
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
Copy Markdown
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.

Comment thread go.mod Outdated
@timburks

timburks commented Aug 4, 2023

Copy link
Copy Markdown
Contributor

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

@pkwarren

pkwarren commented Aug 4, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Contributor Author

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

@timburks

Copy link
Copy Markdown
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.

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