Remove hawtio dependency #316

TheRealJon · 2018-03-12T20:40:58Z

Breaking Changes Remove hawtio-core dependency. A pared down version of hawtio-core is now bundled with the dist scripts. Once this is approved and published, catalog and console will need to be adjusted to properly consume the pluginLoader provided by this repo.

openshift-ci-robot · 2018-03-13T17:30:23Z

New changes are detected. LGTM label has been removed.

benjaminapetersen · 2018-03-13T19:42:44Z

Gruntfile.js

@@ -36,25 +36,51 @@ module.exports = function (grunt) {
      },
      concat: {
        options: {
-          separator: ';'
+          separator: '\n'


I think since we are post-processing/minifying our js, we want to keep this separator as a ;, just in case. I think its a guard against someone forgetting to end a file with a semi, though a \n may solve the same problem.

Can we do something like ";\n"? When looking at the resulting concatenated files, at least one newline made it easier to see where one file ended and the next started. I was inspecting the results of the build and they were just a little difficult to read sometimes.

Yup, that works for me.

benjaminapetersen · 2018-03-13T19:47:10Z

Gruntfile.js

        },
        ui: {
-          src: ['src/**/*UI.module.js', 'dist/scripts/templates.js', 'src/components/**/*.js', 'src/filters/**/*.js', 'src/ui-services/**/*.js'],
+          src: [
+            'src/pluginLoader.js',


Pondering whether pluginLoader fits better under services over ui, or if it should be in both. We consume the dist, so I don't think it can break the console, loading it twice probably overwrites one version, but its hard to tell if this thing has any internal caches that would cause memory leaks.

From what I could tell, these files are intended to be consumed individually and not together. If you only need UI components, you don't have to consume the Services file and vice versa, and if you need both you just consume the origin-web-common.js file.

The other option would be to just leave it out of the concatenated files, then leave it to the consumer to load the pluginLoader script before the others.

benjaminapetersen · 2018-03-13T19:55:29Z

test/karma.conf.js

-      "bower_components/hawtio-extension-service/dist/hawtio-extension-service.js",
-      // load up the fixtures first
+
+      // load fixtures before source


Did we mean to drop kubernetes-container-terminal/dist/container-terminal.js?

Yes, I ran this by Sam. It is not used in this repo. I think it was a copy/paste error at some point.

OK great, good to drop it then.

benjaminapetersen · 2018-03-13T19:56:18Z

test/karma.conf.js

@@ -57,13 +64,13 @@ module.exports = function(config) {
    },

    // web server port
-    port: 9443,
+    port: 4443,


port change needed?

I think someone else changed the port in another PR, but it was 8443 before, which conflicted with the api server if it was running when you try to run tests.

benjaminapetersen · 2018-03-13T19:58:19Z

src/pluginLoader.js

+    });
+    return BootstrapPlugin;
+})();
+


So in the end, should we still name this thing hawtioPluginLoader, or leave it globally as pluginLoader or openshiftPluginLoader && manually update all the references? Thinking on this a second. I don't feel strongly either way atm.

This was a suggestion from Sam to avoid breaking downstream repos other than our own.

My original plan was just to use the global pluginLoader variable and refactor in console and catalog. We can still do that, but the alias still might be necessary.

Works for me then! @spadgett advice tends to be spot on!

Yeah, we documented hawtioPluginLoader.addModule as the way to add modules, so we have to keep that working to avoid breaking customer extensions. I'm OK renaming and fixing up our code as long as we keep it as an alias (with a comment explaining).

https://docs.openshift.org/latest/install_config/web_console_customization.html

I'll add a comment to the LOC where I create the alias. I can also refactor to use 'pluginLoader' in our repos if we want to do that.

benjaminapetersen

Just a few small comments, mainly need to know about that container-terminal script.

spadgett

Thanks @TheRealJon

I wish we could remove more from plugin loader, but I guess it's working and we should leave well enough alone.

spadgett · 2018-03-16T16:48:55Z

src/pluginLoader.js

+  'use strict';
+
+  Logger.setLevel(Logger.INFO);
+  Logger.storagePrefix = 'hawtio';


Hm, I don't see storagePrefix documented in js-logger or used anywhere below. Can we just remove?

I'll try removing and make sure nothing breaks.

spadgett · 2018-03-16T16:50:27Z

src/pluginLoader.js

+    }
+    // console.log("Using log level: ", logLevel);
+    Logger.setLevel(logLevel);
+    if ('showLog' in window.localStorage) {


We're not using showLog so this can be removed.

spadgett · 2018-03-16T16:53:19Z

src/pluginLoader.js

+    var answer = Logger.oldGet(name);
+    Logger.loggers[name] = answer;
+    return answer;
+  };


I think we can remove lines 18-24. I don't see where Logger.loggers is ever used.

spadgett · 2018-03-16T16:54:01Z

src/pluginLoader.js

+    } else {
+      window.localStorage['logBuffer'] = window['LogBuffer'];
+    }
+    if ('childLoggers' in window.localStorage) {


Let's remove the childLoggers stuff. We're not using it.

spadgett · 2018-03-16T16:57:45Z

src/pluginLoader.js

+        }
+      }
+    };
+  }


Let's delete lines 70-97 and use js-logger default handler to add the context name. (I know this is existing hawtio code.)

https://github.com/jonnyreeves/js-logger/blob/46f9df7b946d50574601bb394e5d39c9ebcea338/README.md#default-log-handler-function

spadgett · 2018-03-16T16:58:38Z

src/pluginLoader.js

+
+  Logger.isString = function(obj) {
+    return obj && Logger.getType(obj) === 'String';
+  };


I think we can remove 99-118. I'd like to use vanilla js-logger as much as possible.

spadgett · 2018-03-16T16:59:57Z

src/pluginLoader.js

+      }
+    }
+    onAdd();
+    postLog.forEach(function (func) { func(); });


In fact, I think we can delete 120-250 also. Doesn't look like we're using any of it, and I'd rather not carry the code.

spadgett · 2018-03-16T17:00:29Z

src/pluginLoader.js

+   * plugins from
+   * @type {Array}
+   */
+  // self.urls = [];


Let's remove this since it's commented out anyway.

spadgett · 2018-03-16T17:10:47Z

src/pluginLoader.js

+    return BootstrapPlugin;
+})();
+
+var hawtioPluginLoader = pluginLoader;


Let's make it explicitly window.hawtioPluginLoader = pluginLoader so we're sure it's always a global.

benjaminapetersen · 2018-03-16T19:02:51Z

@spadgett had similar feelings... if it works, & we can drop the dependency, I wasn't sure if we wanted to gut it more or not. I'd kind of like to not see Hawtio obscuring our console.log statements anymore.

spadgett · 2018-03-19T12:34:20Z

Thanks @TheRealJon, looks good. Is this ready? Do you mind squashing?

TheRealJon · 2018-03-19T13:47:45Z

@spadgett Ready for merge.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 12, 2018

TheRealJon requested review from spadgett, benjaminapetersen and dtaylor113 March 12, 2018 20:41

TheRealJon mentioned this pull request Mar 12, 2018

Remove hawtio-core dependency openshift/origin-web-catalog#661

Merged

TheRealJon force-pushed the remove-hawtio branch from 6c1a7a7 to 06f9bd1 Compare March 13, 2018 17:30

benjaminapetersen reviewed Mar 13, 2018

View reviewed changes

TheRealJon force-pushed the remove-hawtio branch from 06f9bd1 to 79079b4 Compare March 13, 2018 20:27

TheRealJon changed the title ~~[WIP] Remove hawtio dependency~~ Remove hawtio dependency Mar 15, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2018

TheRealJon force-pushed the remove-hawtio branch 2 times, most recently from 9e98982 to 5dc7db8 Compare March 16, 2018 16:34

spadgett requested changes Mar 16, 2018

View reviewed changes

spadgett reviewed Mar 16, 2018

View reviewed changes

spadgett approved these changes Mar 19, 2018

View reviewed changes

TheRealJon force-pushed the remove-hawtio branch from 548c8bf to c335420 Compare March 19, 2018 13:15

Remove hawtio dependency

0c4408d

TheRealJon force-pushed the remove-hawtio branch from c335420 to 0c4408d Compare March 19, 2018 16:31

spadgett merged commit bdeada8 into openshift:master Mar 19, 2018

TheRealJon deleted the remove-hawtio branch March 12, 2019 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove hawtio dependency #316

Remove hawtio dependency #316

TheRealJon commented Mar 12, 2018 •

edited

Loading

openshift-ci-robot commented Mar 13, 2018

benjaminapetersen Mar 13, 2018

TheRealJon Mar 13, 2018 •

edited

Loading

benjaminapetersen Mar 13, 2018

benjaminapetersen Mar 13, 2018

TheRealJon Mar 13, 2018

TheRealJon Mar 13, 2018

benjaminapetersen Mar 13, 2018

TheRealJon Mar 13, 2018

benjaminapetersen Mar 13, 2018

benjaminapetersen Mar 13, 2018

TheRealJon Mar 13, 2018

benjaminapetersen Mar 13, 2018

TheRealJon Mar 13, 2018

TheRealJon Mar 13, 2018 •

edited

Loading

benjaminapetersen Mar 13, 2018

spadgett Mar 16, 2018

TheRealJon Mar 16, 2018

benjaminapetersen left a comment

spadgett left a comment

spadgett Mar 16, 2018

TheRealJon Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

spadgett Mar 16, 2018

benjaminapetersen commented Mar 16, 2018

spadgett commented Mar 19, 2018

TheRealJon commented Mar 19, 2018

Remove hawtio dependency #316

Remove hawtio dependency #316

Conversation

TheRealJon commented Mar 12, 2018 • edited Loading

openshift-ci-robot commented Mar 13, 2018

Choose a reason for hiding this comment

TheRealJon Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen left a comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Mar 16, 2018

spadgett commented Mar 19, 2018

TheRealJon commented Mar 19, 2018

TheRealJon commented Mar 12, 2018 •

edited

Loading

TheRealJon Mar 13, 2018 •

edited

Loading

TheRealJon Mar 13, 2018 •

edited

Loading