Skip to content

Conversation

@yevgenypats
Copy link
Contributor

This will let us make breaking changes to the protocol while ensuring users get the right message on what todo. e.g update plugin or cli (client).

This is needed in general and also is a pre-requisite for the performance issue in destination plugins that will require breaking change in the destination protocol.

@github-actions github-actions bot added the feat label Oct 14, 2022
@yevgenypats yevgenypats requested review from disq and hermanschaaf and removed request for disq October 14, 2022 12:03
This will let us make breaking changes to the protocol
while ensuring users get the right message on what todo. e.g
update plugin or cli (client).
@yevgenypats yevgenypats force-pushed the feat/support_protcol_version branch from 2a446a7 to 105a437 Compare October 14, 2022 12:37
return nil, err
}
if protocolVersion < versions.SourceProtocolVersion {
return nil, fmt.Errorf("destination plugin protocol version %d is lower than client version %d. Try updating client", protocolVersion, versions.SourceProtocolVersion)
Copy link
Member

Choose a reason for hiding this comment

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

typo: source

Copy link
Member

@disq disq left a comment

Choose a reason for hiding this comment

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

Looks like a good first step. Rejecting versions and just suggesting upgrading plugins (and for example never suggesting upgrading the cli) won't cover all cases though. In 0.23.0 we had "module version negotiation" where each module responded with a list of versions it supports, and the cli chose the maximum common version. Something like that might be in the cards for the future for here too.

@yevgenypats
Copy link
Contributor Author

yevgenypats commented Oct 14, 2022

Looks like a good first step. Rejecting versions and just suggesting upgrading plugins (and for example never suggesting upgrading the cli) won't cover all cases though. In 0.23.0 we had "module version negotiation" where each module responded with a list of versions it supports, and the cli chose the maximum common version. Something like that might be in the cards for the future for here too.

agree it might be possible future improvement but also I actually don't think more then that is needed. Because in v0 with go-plugin and with protocol negotiation we had so much bugs that most of the time it even didn't print the correct version, so I would just start with a correct message that always work and see how critical it is to support multiple versions in the CLI.

The solution will be not in the SDK anyway because it will be on the cli version to try and use client for older version or for newer version but that's too much for now.

@kodiakhq kodiakhq bot merged commit 3e1492b into main Oct 14, 2022
@kodiakhq kodiakhq bot deleted the feat/support_protcol_version branch October 14, 2022 14:53
kodiakhq bot pushed a commit that referenced this pull request Oct 14, 2022
🤖 I have created a release *beep* *boop*
---


## [0.13.8](v0.13.7...v0.13.8) (2022-10-14)


### Features

* Support application level protocol message. ([#294](#294)) ([3e1492b](3e1492b))


### Bug Fixes

* **tests:** Parallel plugin testing, remove old faker ([#292](#292)) ([48f953a](48f953a))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit that referenced this pull request Oct 16, 2022
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.

3 participants