Compatibility with Kibana 7.14 #235

cpiment · 2021-12-12T14:55:35Z

Partially solves #197

Only needed changes in courier.ts file this time. I have made some tests and everything seems to be working...

Sync master with latest changes

…get hits for document table

fbaligand · 2021-12-13T15:44:01Z

Great!
I will review and test it!

cpiment · 2021-12-15T17:57:35Z

If you need some help with the testing please let me know! I think that testing is harder than coding in this case and maybe I can help with that too so you are not by yourself.

fbaligand · 2021-12-16T09:00:41Z

Hi,

Well, I already checked the code, that is clean.
I made several tests in dev mode, and it works pretty well! I didn't notice any bug.

But when I run yarn build, I have a weird error, associated to 'discover' plugin (that is far away from enhanced-table plugin).

         │          ERROR in C:/kibana-7.14/src/plugins/discover/public/application/apps/main/components/layout/discover_layout.scss?v7dark (C:/kibana-7.14/node_modules/css-loader/dist/cjs.js??ref--6-oneOf-0-1!C:/kibana-7.14/node_modules/postcss-loader/src??ref--6-oneOf-0-2!C:/kibana-7.14/node_modules/sass-loader/dist/cjs.js??ref--6-oneOf-0-3!C:/kibana-7.14/src/plugins/discover/public/application/apps/main/components/layout/discover_layout.scss?v7dark)
         │          Module build failed (from C:/kibana-7.14/node_modules/sass-loader/dist/cjs.js):
         │          SassError: File to import not found or unreadable: src/core/public/mixins.
         │                  on line 3 of C:\kibana-7.14\src\plugins\discover\public\application\apps\main\components\layout\discover_layout.scss
         │          >> @import 'src/core/public/mixins';
         │
         │             ^
         │
         │          SassError: SassError: File to import not found or unreadable: src/core/public/mixins.
         │                  on line 3 of C:\kibana-7.14\src\plugins\discover\public\application\apps\main\components\layout\discover_layout.scss
         │          >> @import 'src/core/public/mixins';
         │
         │             ^

But when I look to stack trace, I see that:

         │           @ C:/kibana-7.14/src/plugins/visualizations/public/index.ts
         │           @ ./public/enhanced-table-vis.js
         │           @ ./public/plugin.ts
         │           @ ./public/index.ts
         │           @ C:/kibana-7.14/node_modules/@kbn/optimizer/target_node/worker/entry_point_creator.js

So the problems starts from enhanced-table.
I looked at public/enhanced-table-vis.js, I clearly don't see any problem or dependency from an external SCSS file from kibana core. The only import is:
import { VIS_EVENT_TO_TRIGGER } from '../../../src/plugins/visualizations/public';

So I did a full compare between core "Data Table" plugin v7.12 and v7.14 (kibana/src/plugins/vis_type_table).
I see a first change in vis_controller.ts (a try/catch block). This not tied to this build problem (I tested) but please can you bring this change, so that runtime errors are better managed?

Then can you tell me if you reproduce my issue when you launch "yarn build"?

fbaligand · 2021-12-17T07:49:06Z

Good news: using yarn compile task, I can generate target folder.
Doing so, I can then generate package for myself, and install it on a standalone Kibana instance.
I did that and then did a full functional test coverage. Everything works great!

So it is finally not so annoying that yarn build does not work.

cpiment · 2021-12-17T07:52:53Z

I have had not time yet to test this, but I would like to try and fix the Kibana yarn build and also implement the changes of vis_controller.ts that you suggested. I will get into it ASAP.

fbaligand · 2021-12-17T12:54:27Z

Honestly, I'm quite pessimistic concerning yarn build.
I don't think that the problem is caused by enhanced-table plugin.
I think that it is tied to kibana core.
can you push the commit on vis_controller.ts?

cpiment · 2021-12-18T16:10:48Z

I have also reproduced the yarn build error in my environment. It's strange because the missing file src/core/public/mixins is nowhere to be found anywhere in the Kibana folder.

The PR that changed the discover plugin is this: elastic/kibana#96766 and it talks about removing angular from the discover plugin, so I think it is related to the build problem because there is a reference to an angular file in the stack trace of the error...

Maybe we can open a bug in Kibana, but since they are trying to get rid of all angular code I think they will not be able to help.

I also added the try/catch you requested.

fbaligand · 2021-12-19T14:02:16Z

Hi @cpiment,

Well, I completely understand that Kibana core aims to remove all its dependencies to AngularJS framework.
But, when a plugin (whatever its under-hood framework) imports a simple class from Kibana core public API , it's not normal that it makes compilation fail (whereas it works fine in dev mode) because of a weird error in discover plugin (whereas enhanced-table plugin has no relation with discover plugin).
So up to me, this is a Kibana core bug.
That said, I hope that this issue has been fixed in a more recent Kibana version.
And especially, I am reassured to know that there is a workaround that makes this issue, not a blocking issue.

fbaligand · 2021-12-19T16:14:11Z

public/vis_controller.ts

@@ -101,6 +102,9 @@ export function getEnhancedTableVisualizationController(
        } else {
          updateScope();
        }
+      } catch (error) {
+        reject(error);
+        }


could you indent nicely try/catch block and inside lines?

I committed a new version with improved indentation, would you mind to review it?

cpiment · 2021-12-19T21:42:29Z

Hi @cpiment,

Well, I completely understand that Kibana core aims to remove all its dependencies to AngularJS framework. But, when a plugin (whatever its under-hood framework) imports a simple class from Kibana core public API , it's not normal that it makes compilation fail (whereas it works fine in dev mode) because of a weird error in discover plugin (whereas enhanced-table plugin has no relation with discover plugin). So up to me, this is Kibana core bug. That said, I hope that this issue has been fixed in a more recent Kibana version. And especially, I am reassured to know that there is a workaround that makes this issue, not a blocking issue.

I have reproduced the issue importing the VIS_EVENT_TO_TRIGGER into the kbn_tp_custom_visualization plugin that is inside the test/plugin_functional/plugins. Without the import I can call build successfully but if I make that import it fails in the same way as your plugin, so as you said this seems to be something not related to your plugin. I'm going to upload the modified test plugin to a repository, check the behavior in 7.15 and 7.16 versions too and open a bug in Kibana repo. I will keep you posted.

fbaligand · 2021-12-19T22:56:48Z

Thanks for all the tests you did.
it's reassuring to know that the issue is not because of enhanced-table plugin code.

Finally, thanks for your following tests and opening an issue on Kibana repository.

fbaligand · 2021-12-19T23:24:23Z

public/vis_controller.ts

+            updateScope();
+          }
+        } catch (error) {
+            reject(error);


Can you just remove 2 spaces on line 106 please?
And then, it will be OK for me. I will merge the PR.

I committed a new version with this change :)

fbaligand · 2021-12-20T08:18:56Z

Thanks for this last change => LGTM!

cpiment · 2021-12-20T22:23:33Z

Thanks for all the tests you did. it's reassuring to know that the issue is not because of enhanced-table plugin code.

Finally, thanks for your following tests and opening an issue on Kibana repository.

I have just filed the issue elastic/kibana#121713.

While testing with my little test plugin I have confirmed that the error doesn't reproduce using Kibana 7.15.0 nor 7.15.1 source code, so maybe we can build the 7.14.2 plugin using the 7.15 source code, would that be possible?

fbaligand · 2021-12-20T23:09:33Z

Hi @cpiment,

Nice to see that you don't reproduce the issue with Kibana 7.15!
By the way, if you don't reproduce the issue on Kibana 7.15, I'm afraid that your issue will never be processed.
Elastic doesn't maintain past minor releases. They maintain only last minor version (here 7.16) and previous major version (here 6.8).

That said, I just released enhanced-table v1.12.1 with Kibana 7.14 support!
https://github.com/fbaligand/kibana-enhanced-table/releases/tag/v1.12.1

I have a custom package script that first generate "target" folder (using yarn compile) then build the package for every Kibana 7.14 versions. This has the advantage to be really fast compared to call yarn build for every Kibana patch.
And so, that's why yarn build broken is not a blocking issue, and why I don't need to use Kibana 7.15 source code.

I also made tests with Kibana 7.15 (standalone) to see if the same code is compatible, and sadly, there are breaking changes :(
So a new migration work will be needed.

cpiment · 2021-12-21T14:30:41Z

Opened PR #236 with the changes to make the plugin with 7.15.

Thanks for reviewing all this @fbaligand !!

cpiment added 6 commits October 21, 2021 15:46

WIP: Kibana 7.12

31006e8

Sync with upstream master

be5eb38

Merge pull request #1 from fbaligand/master

cf95234

Sync master with latest changes

Fixed enhanced table, document table does not work

278cd15

modified courier.ts to reflect 7.14 changes and add modifications to …

6c71166

…get hits for document table

removed IDE files

d920698

added try/catch block in method

07b48fe

fbaligand reviewed Dec 19, 2021

View reviewed changes

Fixed indentation in try/catch block

5f332ab

fbaligand reviewed Dec 19, 2021

View reviewed changes

Fixed indentation in catch block of vis_controller.ts

815900e

fbaligand merged commit 37dd1f2 into fbaligand:master Dec 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatibility with Kibana 7.14 #235

Compatibility with Kibana 7.14 #235

cpiment commented Dec 12, 2021

fbaligand commented Dec 13, 2021

cpiment commented Dec 15, 2021

fbaligand commented Dec 16, 2021

fbaligand commented Dec 17, 2021

cpiment commented Dec 17, 2021

fbaligand commented Dec 17, 2021

cpiment commented Dec 18, 2021 •

edited

Loading

fbaligand commented Dec 19, 2021 •

edited

Loading

fbaligand Dec 19, 2021

cpiment Dec 19, 2021

cpiment commented Dec 19, 2021

fbaligand commented Dec 19, 2021

fbaligand Dec 19, 2021

cpiment Dec 20, 2021

fbaligand commented Dec 20, 2021

cpiment commented Dec 20, 2021

fbaligand commented Dec 20, 2021

cpiment commented Dec 21, 2021

Compatibility with Kibana 7.14 #235

Compatibility with Kibana 7.14 #235

Conversation

cpiment commented Dec 12, 2021

fbaligand commented Dec 13, 2021

cpiment commented Dec 15, 2021

fbaligand commented Dec 16, 2021

fbaligand commented Dec 17, 2021

cpiment commented Dec 17, 2021

fbaligand commented Dec 17, 2021

cpiment commented Dec 18, 2021 • edited Loading

fbaligand commented Dec 19, 2021 • edited Loading

fbaligand Dec 19, 2021

Choose a reason for hiding this comment

cpiment Dec 19, 2021

Choose a reason for hiding this comment

cpiment commented Dec 19, 2021

fbaligand commented Dec 19, 2021

fbaligand Dec 19, 2021

Choose a reason for hiding this comment

cpiment Dec 20, 2021

Choose a reason for hiding this comment

fbaligand commented Dec 20, 2021

cpiment commented Dec 20, 2021

fbaligand commented Dec 20, 2021

cpiment commented Dec 21, 2021

cpiment commented Dec 18, 2021 •

edited

Loading

fbaligand commented Dec 19, 2021 •

edited

Loading