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

Check for SDK compatibility against current Kuzzle version #1495

Merged

Conversation

Aschen
Copy link
Contributor

@Aschen Aschen commented Oct 15, 2019

What does this PR do ?

v1: #1494

Now Kuzzle will check the volatile.sdkName header to ensure that the current SDK is compatible against the current Kuzzle version.

For Kuzzle v1 SDKs, the sdkVersion property is present. So when the property is present, we assume that the SDK is incompatible.

For Kuzzle v2 SDKs, a compatibility matrix has been added. It's checking the volatile.sdkVersion property and works with simplified semver ranges.

Example compat matrix:

{
  "js": ">=7" // this version of Kuzzle is compatible with the JS SDKs beginning at the 7.x.x version
}

Example sdkName volatile property: js@7.4.2

Other changes

  • upgrade Elasticsearch version to 7.4.0

@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #1495 into 2-dev will increase coverage by 0.02%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##            2-dev    #1495      +/-   ##
==========================================
+ Coverage   92.82%   92.85%   +0.02%     
==========================================
  Files          98       97       -1     
  Lines        6678     6621      -57     
==========================================
- Hits         6199     6148      -51     
+ Misses        479      473       -6
Impacted Files Coverage Δ
lib/api/controllers/funnelController.js 95.76% <96.87%> (+0.07%) ⬆️
lib/api/core/entrypoints/index.js 98.88% <0%> (-0.01%) ⬇️
lib/api/core/janitor.js 89% <0%> (ø) ⬆️
lib/api/core/entrypoints/protocols/socketio.js
lib/api/core/plugins/pluginsManager.js 89.62% <0%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b72934...cc83c9b. Read the comment docs.

lib/api/controllers/funnelController.js Outdated Show resolved Hide resolved
lib/api/controllers/funnelController.js Outdated Show resolved Hide resolved
*
* @returns {Boolean}
*/
function satisfiesMajor (version, range) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using semver, which is already included in our package.json?

The way it's currently coded, the compatibility matrix is misleading. I, for one, would have expected a semver range, and I expect to be able to enter something like >=7 <9 for instance, which is a probable value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I choose to use some kind of simplified semver range for now because semver is really slow and this method is called whenever Kuzzle receive a request.

Semver x 176,414 ops/sec ±4.15% (85 runs sampled)
Manual: only major x 776,075,210 ops/sec ±2.42% (90 runs sampled)

We won't need support for this kind of range (>=7 <9) until v3.

Do you think it's unnecessary ?

Copy link
Contributor

@scottinet scottinet Oct 16, 2019

Choose a reason for hiding this comment

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

Indeed, this is slow.

My real problem is the syntax similarity with semver, I find that very misleading for future maintenance.

Since the matrix is internal, I think that using a custom format that has nothing to do with semver might be a better idea.

Something like:

{
  "js": { "min": 7 },
  "csharp": { "max": 2},
  "cpp": { "max": 2},
  "java": { "max": 3},
  "android": { "max": 5},
  "go": { "max": 3},
  "php": { "max": 4}
}

Of course, we would be able to have both a min and a max value. This should make the version check even faster while unambiguously being different from semver.

What do you think?

Copy link
Contributor

@scottinet scottinet left a comment

Choose a reason for hiding this comment

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

you need to remove the auth-local plugin update from this PR

apart from that: 👍

@Aschen Aschen requested a review from scottinet October 22, 2019 09:08
@Aschen
Copy link
Contributor Author

Aschen commented Oct 22, 2019

you need to remove the auth-local plugin update from this PR

apart from that: +1

Done @scottinet

@Aschen Aschen merged commit 5d58e5d into 2-dev Nov 1, 2019
@Aschen Aschen deleted the KZL-1380/v2-check-request-volatile-for-sdk-version-compatibility branch November 1, 2019 09:55
@Aschen Aschen mentioned this pull request Nov 5, 2019
Aschen added a commit that referenced this pull request Nov 5, 2019
# [2.0.0-rc.2](https://github.com/kuzzleio/kuzzle/releases/tag/2.0.0-rc.2) (2019-11-05)


#### Breaking changes

- [ [#1500](#1500) ] Remove Dsl constructor from plugin context   ([Aschen](https://github.com/Aschen))
- [ [#1489](#1489) ] Drop support for socket.io   ([scottinet](https://github.com/scottinet))
- [ [#1491](#1491) ] Remove strategy constructors support   ([Aschen](https://github.com/Aschen))
- [ [#1476](#1476) ] Change Document controller m* routes returns & fix SDK functional tests   ([Aschen](https://github.com/Aschen))

#### Bug fixes

- [ [#1512](#1512) ] Fix subscribing with a dead connection   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1507](#1507) ] Properly destroy a user after a credentials creation failure   ([scottinet](https://github.com/scottinet))
- [ [#1483](#1483) ] Fix socketio connections leak & deactivate this protocol by default   ([scottinet](https://github.com/scottinet))

#### New features

- [ [#1502](#1502) ] Add a new "document:exists" API route   ([scottinet](https://github.com/scottinet))
- [ [#1501](#1501) ] Add collection delete   ([Aschen](https://github.com/Aschen))
- [ [#1499](#1499) ] Expose an Elasticsearch client constructor in plugin context   ([Aschen](https://github.com/Aschen))
- [ [#1484](#1484) ] Upgrade script   ([scottinet](https://github.com/scottinet))

#### Enhancements

- [ [#1510](#1510) ] Mapping - enable dynamic templates   ([benoitvidis](https://github.com/benoitvidis))
- [ [#1495](#1495) ] Check for SDK compatibility against current Kuzzle version   ([Aschen](https://github.com/Aschen))
- [ [#1498](#1498) ] Add highlight to search results   ([Aschen](https://github.com/Aschen))
- [ [#1488](#1488) ] Add kuzzle version to the dumped informations   ([scottinet](https://github.com/scottinet))
- [ [#1481](#1481) ] Error codes normalization   ([scottinet](https://github.com/scottinet))
- [ [#1478](#1478) ] Error codes redux   ([scottinet](https://github.com/scottinet))
---
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.

None yet

2 participants