-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update protobuf #7411
Update protobuf #7411
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit fc449e5:
|
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.
a couple optional suggestions, plus fix smoke test (we looked into it separately and it seems to be due to older versions of Gateway depending on apollo-usage-reporting-protobuf
which should also get updated first)
@@ -19,6 +19,7 @@ dictionaries: | |||
useGitignore: true | |||
ignorePaths: | |||
- '**/generated/**' | |||
- 'packages/usage-reporting-protobuf/src/reports.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.
I guess flipside here is, if there are typos in this file then we should fix them at the source? (Perhaps with a temporary addition to local dictionary?)
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.
Eh, the word is nestedness
which seems reasonable / not something I'd fix over there. I do think this falls into the "generated" category and shouldn't be spell checked here going forward, but not opposed to just adding to dictionary too.
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.
Maybe we do want to permit nestedness
and catch future spelling problems here 🤷
The protobuf doesn't actually need to be updated, however it is causing compatibility issues with the protobuf update on `main` given that we're smoke testing against old versions of the gateway as well which depend on the v3 version of the `apollo-reporting-protobuf` package. Related: #7411
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @apollo/usage-reporting-protobuf@4.1.0 ### Minor Changes - [#7411](#7411) [`021460e95`](021460e) Thanks [@trevor-scheer](https://github.com/trevor-scheer)! - Update protobuf which includes updates for supporting (notably) ConditionNode in the gateway ## @apollo/server-integration-testsuite@4.4.1 ### Patch Changes - [#7381](#7381) [`29038a4d3`](29038a4) Thanks [@renovate](https://github.com/apps/renovate)! - Update graphql-http dependency - Updated dependencies \[[`021460e95`](021460e)]: - @apollo/usage-reporting-protobuf@4.1.0 - @apollo/server@4.4.1 ## @apollo/server@4.4.1 ### Patch Changes - Updated dependencies \[[`021460e95`](021460e)]: - @apollo/usage-reporting-protobuf@4.1.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Update protobuf to latest, which notably introduces a
ConditionNode
needed to reintroduce support for@skip
/@include
in gateway traces.These changes were produced by running the following: