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

chore(weave): Simplify TraceServerInterface definition and fix inheritance #1763

Merged
merged 6 commits into from
Jun 12, 2024

Conversation

tssweeney
Copy link
Contributor

@tssweeney tssweeney commented Jun 12, 2024

We introduced a violation of inheritance rules with TraceServerInterfacePostAuth. We added this because the interface definition for the HTTP server differs slightly from that of the internal trace storage layers (namely the inclusion of wb_user_id which is populated by the server - and therefore not required).

This was problematic for a few reasons:

  1. Most importantly: it made it so that RemoteHTTPTraceServer was no longer compatible with the other servers. This is not a good place to be in.
  2. It violates inheritance rules
  3. It introduces additional complexity (not that big of a deal

To remedy this, I:

  1. Removed the TraceServerInterfacePostAuth
  2. Added runtime checks to assert the presence of the required field in the implementations which require it.

@circle-job-mirror
Copy link

circle-job-mirror bot commented Jun 12, 2024

@circle-job-mirror
Copy link

@tssweeney tssweeney marked this pull request as ready for review June 12, 2024 20:45
@tssweeney tssweeney requested a review from a team as a code owner June 12, 2024 20:45
Copy link
Member

@gtarpenning gtarpenning left a comment

Choose a reason for hiding this comment

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

lgtm, @jamie-rasmussen might want to 👀 too though

@tssweeney tssweeney merged commit d4ba75d into master Jun 12, 2024
24 checks passed
@tssweeney tssweeney deleted the tim/split_interfaces branch June 12, 2024 22:28
@github-actions github-actions bot locked and limited conversation to collaborators Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants