Adds concept of kibanaVersion to plugin installer #8222

BigFunger · 2016-09-12T15:12:23Z

The plugin installer checks the package.json file of the plugin in the archive. It compares the version property defined there against the version property defined in Kibana's package.json file.
If the two do not match down to the patch level, the plugin will not be installed. (ignoring everything after a hyphen in the comparison)

This PR keeps that functionality the same.

Now, however, the installer looks for a explicitly defined kibana.version property and uses that in the comparison if it exists. It falls back to the version property if kibana.version is not defined.

Example

{
  "version": "1.0.4"
}

will result in the plugin installer comparing 1.0.4 during the version check against the Kibana version.

{
  "version": "1.0.4",
  "kibana": {
    "version": "5.0.1"
  }
}

will result in the plugin installer comparing 5.0.1 during the version check against the Kibana version.

cjcenizal · 2016-09-12T20:05:37Z

I'm working on some similar version-comparison logic, but the rules for satisfying the version requirements are slightly more complex. Because of that, I don't think it's worth reusing my code here, but I thought I should cross-link anyway in case you're interested: #8229

epixa · 2016-09-12T20:11:08Z

@cjcenizal Yeah, we draw a much harder line for plugin versions than we do with ES/Kibana since we don't necessarily control the code of the plugin itself, so we can't make assumptions about compatibility.

cjcenizal · 2016-09-12T20:13:26Z

src/cli_plugin/install/kibana.js

@@ -46,14 +46,14 @@ export async function rebuildCache(settings, logger) {
 }

 export function assertVersion(settings) {
-  if (!settings.plugins[0].version) {
+  if (!settings.plugins[0].kibanaVersion) {


This error can also be solved by adding a kibana.version property, right? We should probably update the Error to state something along these lines: "Plugin package.json is missing both a version property (required) and a kibana.version property (optional)."

cjcenizal · 2016-09-12T20:35:35Z

This is looking good! I have a few comments:

__tests__/kibana.js has a typo on line 62: It says "does" but it should say "doesn't".
Same file, typo on line 75: it says "does does" (has an extra word).
Maybe the versionSatisfies function should be renamed to versionsMatch to be more explicit. Without a clearer name, you have to open up that file and read its implementation to understand the requirements we're enforcing.

BigFunger · 2016-09-12T20:44:12Z

I still think that versionSatisfies better describes the functionality than versionsMatch because it ignores all data after the hyphen... And will continue to describe it correctly as our comparison loosens in the future (to be Major/Minor) instead of (Major/Minor/Patch).

cjcenizal · 2016-09-12T21:17:56Z

LGTM!

w33ble · 2016-09-12T21:28:21Z

src/cli_plugin/install/__tests__/kibana.js

@@ -45,7 +45,7 @@ describe('kibana cli', function () {
            tempArchiveFile: tempArchiveFilePath,
            plugin: 'test-plugin',
            version: '5.0.0-SNAPSHOT',
-            plugins: [ { name: 'foo', path: join(testWorkingPath, 'foo'), version: '5.0.0-SNAPSHOT' } ]
+            plugins: [ { name: 'foo', path: join(testWorkingPath, 'foo'), kibanaVersion: '5.0.0-SNAPSHOT' } ]


Shouldn't the tests be checking both version and kibanaVersion? As in, one set for only kibanaVersion and another for version?

This is slightly confusing....

const settings = { workingPath: testWorkingPath, tempArchiveFile: tempArchiveFilePath, plugin: 'test-plugin', version: '5.0.0-SNAPSHOT', plugins: [ { name: 'foo', path: join(testWorkingPath, 'foo'), kibanaVersion: '5.0.0-SNAPSHOT' } ] };

the version property here refers to Kibana's actual version as defined by kibana's package.json

So apparently kibanaVersion is derived, and I should have spent more time digging into the changes here. So this is fine.

w33ble · 2016-09-12T22:15:28Z

Tested with x-plugins:

$ ./bin/kibana-plugin install file:../x-plugins/kibana/target/x-pack-6.0.0-alpha1.zip
Attempting to transfer from file:../x-plugins/kibana/target/x-pack-6.0.0-alpha1.zip
Transferring 68453372 bytes....................
Transfer complete
Retrieving metadata from plugin archive
Extracting plugin archive
Extraction complete
Optimizing and caching browser bundles...
Plugin installation complete

Then added a mismatched kibana.version to the package:

$ ./bin/kibana-plugin install file:../x-plugins/kibana/target/x-pack-6.0.0-alpha1.zip
Attempting to transfer from file:../x-plugins/kibana/target/x-pack-6.0.0-alpha1.zip
Transferring 68453385 bytes....................
Transfer complete
Retrieving metadata from plugin archive
Extracting plugin archive
Extraction complete
Plugin installation was unsuccessful due to error "Incorrect Kibana version in plugin [x-pack]. Expected [6.0.0]; found [7.0.0-failing-test]"

Functionality looks good.

w33ble · 2016-09-12T22:30:12Z

src/cli_plugin/install/pack.js

+    // the version of kibana down to the patch level. If these two versions need
+    // to diverge, they can specify a kibana.version to indicate the version of
+    // kibana the plugin is intended to work with.
+    pkg.kibanaVersion = _.get(packageInfo, 'kibana.version');


You can use pkg.kibanaVersion = _.get(packageInfo, 'kibana.version', pkg.version); and get rid of the check below.

w33ble · 2016-09-12T22:32:26Z

Small change, otherwise LGTM!

backports #8222 to 5.x

epixa · 2016-09-12T22:53:48Z

Backported to 5.x and 5.0, though I forgot to amend the commit on 5.0 to link back up to this PR: a691874

backports elastic#8222 to 5.x Former-commit-id: 3a96d98

Adds concept of kibanaVersion to plugin installer

3b8ff97

BigFunger added the review label Sep 12, 2016

BigFunger mentioned this pull request Sep 12, 2016

Add plugin version check to Kibana startup #6652

Closed

cjcenizal self-assigned this Sep 12, 2016

cjcenizal reviewed Sep 12, 2016
View reviewed changes

BigFunger force-pushed the plugin-installer-kibana-version branch from 99f2737 to 89db806 Compare September 12, 2016 20:45

BigFunger force-pushed the plugin-installer-kibana-version branch from 89db806 to 901fde5 Compare September 12, 2016 21:19

w33ble reviewed Sep 12, 2016
View reviewed changes

w33ble self-assigned this Sep 12, 2016

BigFunger assigned Bargs Sep 12, 2016

w33ble reviewed Sep 12, 2016
View reviewed changes

Added some comments, and clarified error message

29f58a1

BigFunger force-pushed the plugin-installer-kibana-version branch from 901fde5 to 29f58a1 Compare September 12, 2016 22:35

BigFunger merged commit 90ad531 into elastic:master Sep 12, 2016

epixa pushed a commit that referenced this pull request Sep 12, 2016

Added some comments, and clarified error message

3a96d98

backports #8222 to 5.x

epixa added the v5.0.0-beta1 label Sep 12, 2016

BigFunger mentioned this pull request Sep 14, 2016

Check plugin version on startup #8283

Merged

epixa added the v5.0.0 label Oct 26, 2016

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Added some comments, and clarified error message

d469e1d

backports elastic#8222 to 5.x Former-commit-id: 3a96d98

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds concept of kibanaVersion to plugin installer #8222

Adds concept of kibanaVersion to plugin installer #8222

BigFunger commented Sep 12, 2016 •

edited by w33ble

Loading

cjcenizal commented Sep 12, 2016

epixa commented Sep 12, 2016

cjcenizal Sep 12, 2016

cjcenizal commented Sep 12, 2016

BigFunger commented Sep 12, 2016

cjcenizal commented Sep 12, 2016

w33ble Sep 12, 2016

BigFunger Sep 12, 2016 •

edited

Loading

w33ble Sep 12, 2016

w33ble commented Sep 12, 2016

w33ble Sep 12, 2016 •

edited

Loading

BigFunger Sep 12, 2016

w33ble commented Sep 12, 2016

epixa commented Sep 12, 2016

Adds concept of kibanaVersion to plugin installer #8222

Adds concept of kibanaVersion to plugin installer #8222

Conversation

BigFunger commented Sep 12, 2016 • edited by w33ble Loading

Example

cjcenizal commented Sep 12, 2016

epixa commented Sep 12, 2016

cjcenizal Sep 12, 2016

Choose a reason for hiding this comment

cjcenizal commented Sep 12, 2016

BigFunger commented Sep 12, 2016

cjcenizal commented Sep 12, 2016

w33ble Sep 12, 2016

Choose a reason for hiding this comment

BigFunger Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

w33ble Sep 12, 2016

Choose a reason for hiding this comment

w33ble commented Sep 12, 2016

w33ble Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

BigFunger Sep 12, 2016

Choose a reason for hiding this comment

w33ble commented Sep 12, 2016

epixa commented Sep 12, 2016

BigFunger commented Sep 12, 2016 •

edited by w33ble

Loading

BigFunger Sep 12, 2016 •

edited

Loading

w33ble Sep 12, 2016 •

edited

Loading