• fix: use explicit api versions for agent and tailnet #15508 👈(View in Graphite)
• feat: set DNS hostnames in workspace updates controller #15507
• feat: add workspace updates controller #15506
• main

This stack of pull requests is managed byGraphite. Learn more aboutstacking.

Does this PR cover the case where proto changes are introduced without bumping the version? If it is I didn't quite catch how that results in a build failure. (I.e. is anything stopping me from appending a new method to DRPCAgentClient23?)

My assumption is that we just editedagent.proto andtailnet.proto, then ranmake gen and forgot/didn't know we needed to care about the version. So, having a separateDRPCAgentClient23 piped around theagent means that when you go to use your fancy new method, it isn't on the interface and your build will fail. Youcould just add it to the interface manually, but my hope is that the comment on that interface makes it a bit more obvious that this is a versioned interface and you shouldn't do that. Before we were just passing around thedrpc.Conn, and constructing aDRPCAgentClient, which is autogenerated from the.proto, so it was very easy not to realize the protocol is versioned.

@spikecurtis ok, thanks for the explanation. This got me thinking though, could we instead make the protocol version a type alias so you don't need to change all the code every time you're switching? I.e.

typeDRPCAgentClientLatest=DRPCAgentClient23func (c*Client)ConnectRPCLatest(...) {return c.ConnectRPC23(...) }

Edit: Thinking about this some more, this would essentially automatically bump the protocol in the common case for all consumers when you change the alias. Maybe that's not what we want. I was hoping to avoid the yank renaming across the code-base but may be unavoidable without some rethinking of the API.

@spikecurtis ok, thanks for the explanation. This got me thinking though, could we instead make the protocol version a type alias so you don't need to change all the code every time you're switching? I.e.

typeDRPCAgentClientLatest=DRPCAgentClient23func (c*Client)ConnectRPCLatest(...) {return c.ConnectRPC23(...) }

Edit: Thinking about this some more, this would essentially automatically bump the protocol in the common case for all consumers when you change the alias. Maybe that's not what we want. I was hoping to avoid the yank renaming across the code-base but may be unavoidable without some rethinking of the API.

@mafredri I think if you did this, maybe we could do something with golden files and dump a JSON representation of the interface somewhere. If it changes, via a argument field change, new method, etc. The test would fail until youupdate-golden-files again.

Just an idea to "snapshot" what the API is from a JSON POV and dump it into a test that can detect changes.

@spikecurtis I know you mentioned it might not be worth spending a lot of time on this infrastructure. I'll defer to ya'll on it.

maybe we could do something with golden files and dump a JSON representation of the interface somewhere. If it changes, via a argument field change, new method, etc. The test would fail until you update-golden-files again.

Just an idea to "snapshot" what the API is from a JSON POV and dump it into a test that can detect changes.

Look, if we wanted to snapshot the API definition, we could just make a copy of the.proto file. No need to involve JSON.

This isn't like a "golden file" where you're testing that the code gives some complex output that's easier to just compare wholesale rather than asserting individual facts about it.

We could stash a snapshot of the current version in testdata and directly compare, so that if you edited the proto without creating a new version, tests would fail. Personally, I think CODEOWNERS should help here, but if y'all really want another robot to complain to you about stuff, we can do it.

we could just make a copy of the.proto file. No need to involve JSON.

Oh yea 🤦. Nah, this is all fine with me 👍

Let's go with this for now, we can always revisit if the process is too cumbersome or has other shortcomings 👍🏻.

Merge activity

• Nov 15, 2:16 AM EST: A user merged this pull request withGraphite.