Make the id field on ServerMessage::Error optional - as it can be ret…

[jeffutter]

This PR FIXES #6138

The websocket-transport-ws spec does not require an id on a CONNECTION_ERROR. This makes that field optional.

---

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

• PR description explains the motivation for the change and relevant context for reviewing
• PR description links appropriate GitHub/Jira tickets (creating when necessary)
• Changes are compatible[^1]
• Performance impact assessed and acceptable
• Tests added and passing[^4]
  • Unit tests
  • Integration tests
  • Manual tests, as necessary

[abernix]

Heya, thanks for opening this PR!

We'll try to get someone to take a look at it in the next couple days and tentatively slate it for our next patch release. Note, however, that the CI checks on this PR won't pass because this feature requires an license which is only available by providing that locally. The litmus test for whether they'll pass would be if you're able to get new/existing tests passing locally using TEST_APOLLO_KEY and TEST_GRAPH_REF environment variables with appropriate API keys (which have access to a graph with this feature enabled) and graph reference for the same. You'd also want to have run docker compose up from the root of the repository in order to get the Jaeger/OpenTelemetry/Datadog components up and working for the tests.

[jeffutter]

Thanks, in regards to tests, we have a branch where we skip/delete most of the tests that require additional integration. As is 2753 tests pass and none fail. So I'm pretty confident that my added test passes and that we didn't break anything.

[bnjjj]

LGTM but could you just create a changelog using cargo xtask changeset create please ?

[abernix]

@Mergifyio copy dev

[mergify]

copy dev

✅ Pull request copies have been created

Details

• #8189 Make the id field on ServerMessage::Error optional - as it can be ret… (copy #8181) has been created for branch dev

[abernix]

@bnjjj thanks for the review — @jeffutter while we do ask for changesets typically I'll absorb that ask on this one.

To get this over the line:

• i'm going to copy this PR to another PR so we can get the CI run going on it
• Add the changeset myself
• Ensure this is backported and target it for our upcoming 2.6.1 patch release.

I just asked Mergify to make a copy of this PR, and it should appear/be linked relatively soon. It should also preserve the contribution credit, but I'll make sure it does. Thanks again

Edit: new PR is #8189

[abernix]

Closing as this PR is now superseded by #8189.