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

separate library version from feature version #316

Merged
merged 3 commits into from
Dec 15, 2017
Merged

separate library version from feature version #316

merged 3 commits into from
Dec 15, 2017

Conversation

wxing1292
Copy link
Contributor

separate the library version from feature version, also include the client lang in the rpc call to service.

@wxing1292 wxing1292 self-assigned this Dec 14, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 67.887% when pulling 388f4a1 on meta into 16bac02 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 67.835% when pulling 874f1f3 on meta into 5a46218 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 67.955% when pulling 971713a on meta into 5a46218 on master.

// languageHeaderName refers to the name of the
// tchannel / http header that contains the client
// language
languageHeaderName = "cadence-client-language"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename it to cadence-client-name
and value be something like uber-go, uber-java, as someone else can create mylibrary-java, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be even call it user-agent to match HTTP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to be "cadence-client-name" : "uber-go"

// feature set of this cadence client library support.
// This can be used for client capibility check, for
// backward compatibility
const FeatureVersion = "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should also be semantic version with MAJOR.MINOR version of the server it was tested against. The server itself would have MAJOR.MINOR.PATCH version, where PATCH would be for any changes that are not API changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so what about this:

internally, we keep 3 version number on the client side (major, minor, patch), and when exposing to customer who relying on the version number, we just exposed_version = sprintf("v%s.%s.%s"), i.e. "v1.2.3"

when piggy the version back to server, we send back 3 version separately, like:
"cadence-client-major-ver": "1"
"cadence-client-minor-ver": "2"
"cadence-client-patch-ver": "3"
to avoid parsing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused what version are you talking about?
For feature version only two numbers are necessary, and parsing is trivial.
For client version there is no need for parsing yet.

// by the cadence team as part of a major feature or
// behavior change

// LibraryVersion is a semver string that represents
// the version of this cadence client library
const LibraryVersion = "v0.5.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

This version should represent API changes visibile to Cadence client side library consumers. I.e. developers that are writing workflows. So every time we change API that can affect them we have to change this number. And we can even add code to validate that workflow definition matches it in the future.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 67.955% when pulling 56a2c0d on meta into 5a46218 on master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants