Skip to content

Preserve backwards compatibility of teleport.lib.teleterm.v1.Gateway#26705

Merged
ravicious merged 1 commit intomasterfrom
ravicious/gateway-proto
May 23, 2023
Merged

Preserve backwards compatibility of teleport.lib.teleterm.v1.Gateway#26705
ravicious merged 1 commit intomasterfrom
ravicious/gateway-proto

Conversation

@ravicious
Copy link
Copy Markdown
Member

#26441 introduced what is technically a breaking change in teleterm's protos by changing the type of a field while reusing its name and number. This tripped up the CI check that Edoardo is working on (#26441 (comment)).

This PR corrects this by reserving the old field name and number and adding a new field with the new type.

@ravicious ravicious requested review from gzdunek and removed request for ibeckermayer May 22, 2023 16:59
@ravicious
Copy link
Copy Markdown
Member Author

Oops, I forgot to change the Go code. 🤦

@ravicious
Copy link
Copy Markdown
Member Author

For the Go code I changed only what's necessary to make the code compile. The Go code talks about "CLI command" much more, which kinda makes sense in that case since it operates on os.exec.Cmd. While "CLI command" as a name in general makes less sense (command-line interface command), I think I'll just leave it like that for now.

Copy link
Copy Markdown
Contributor

@ibeckermayer ibeckermayer left a comment

Choose a reason for hiding this comment

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

Granted that I'm not up to speed on what's going on in this section of the code, but at first glance a GatewayCLIClient that only sends a single command makes less sense than a GatewayCLICommand. I typically think of a client as a thing that sends and receives messages repeatedly, whereas afaict this object represents just a single command sent once.

I don't quite understand what you think doesn't make sense about "command-line interface command", to me that sounds like "a command that is sent to the command-line interface".

@espadolini
Copy link
Copy Markdown
Contributor

(not entering the merit of how to call the field and the message)

Comment thread proto/teleport/lib/teleterm/v1/gateway.proto Outdated
@ravicious
Copy link
Copy Markdown
Member Author

I hear your concerns @ibeckermayer. It is a single command that's supposed to launch the client. I'm going to use Edoardo's suggestion, gateway_cli_command, which I think addresses your concerns and at the same time doesn't necessitate as many changes in the frontend code as cli_client does.

@ravicious ravicious force-pushed the ravicious/gateway-proto branch from 781ec00 to 643877b Compare May 23, 2023 08:38
@ravicious ravicious enabled auto-merge May 23, 2023 08:52
@ravicious ravicious added this pull request to the merge queue May 23, 2023
Merged via the queue into master with commit daca5bf May 23, 2023
@ravicious ravicious deleted the ravicious/gateway-proto branch May 23, 2023 12:25
@ravicious
Copy link
Copy Markdown
Member Author

Oops, I forgot that I should probably wait for all reviewers before enabling auto merge, I hope all is good @ibeckermayer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants