add cluster icon migration code #673

Nokel81 · 2020-08-11T15:00:45Z

This PR adds a migration to the cluster store so that icon that were previously stored on the file system are instead stored in their base64 encoding within the cluster store.

ixrock

LGTM

src/migrations/cluster-store/3.6.0-beta.1.ts

src/renderer/components/+cluster-settings/components/cluster-icon-setting.tsx

ixrock · 2020-08-11T15:23:08Z

src/migrations/cluster-store/3.6.0-beta.1.ts

+         */
+        try {
+          if (cluster.preferences.icon) {
+            const fileData = fse.readFileSync(cluster.preferences.icon);


Is it was absolute path to icon, right?

I think so, looking at https://github.com/lensapp/lens/blob/v3.5.0/src/main/cluster-manager.ts#L36

Nokel81 · 2020-08-11T20:10:02Z

I don't know the exact problems that using async migration functions might lend themselves to, but it is something that I would like to see changed (I raised sindresorhus/conf#123) to hopefully get the ball rolling.

nevalla · 2020-08-17T13:37:44Z

Had some issues while testing this. Need to figure out is it user error or real issue.

nevalla · 2020-08-19T11:23:41Z

So, back to my issue. The error I got is the following:

STORE MIGRATION (/Users/lauri/Library/Application Support/LensDev/lens-cluster-store.json): 3.6.0-beta.1
[0] Number of clusters to migrate:  1
[0] Failed to migrate cluster icon for cluster "c959e2c4-45dc-4a1a-93fd-dd0494c520eb" Error: ENOENT: no such file or directory, open 'store:///icons/minikube-minikube-logo.png'
[0]     at Object.openSync (fs.js:440:3)
[0]     at Object.func [as openSync] (electron/js2c/asar.js:140:31)
[0]     at Object.readFileSync (fs.js:342:35)
[0]     at Object.fs.readFileSync (electron/js2c/asar.js:542:40)
[0]     at eval (webpack-internal:///./src/migrations/cluster-store/3.6.0-beta.1.ts:52:86)
[0]     at Array.map (<anonymous>)
[0]     at run (webpack-internal:///./src/migrations/cluster-store/3.6.0-beta.1.ts:33:14)
[0]     at 3.6.0-beta.1 (webpack-internal:///./src/migrations/migration-wrapper.ts:14:13)
[0]     at Conf._migrate (/Users/lauri/Projects/lens/node_modules/conf/dist/source/index.js:400:17)
[0]     at new Conf (/Users/lauri/Projects/lens/node_modules/conf/dist/source/index.js:144:18) {
[0]   errno: -2,
[0]   syscall: 'open',
[0]   code: 'ENOENT',
[0]   path: 'store:///icons/minikube-minikube-logo.png'
[0] }

So the icon stored in cluster store is in format of store://... so we can't read it directly, but we need to convert that to real file path. Also found mime-types npm package that offers the same functionality but without promises. With these changes I tried with this:

...
        /**
        * migrate cluster icon
        */
        try {
          if (cluster.preferences.icon) {
            const fileName = path.join((app || remote.app).getPath("userData"), cluster.preferences.icon.replace("store://", ""))
            const fileData = fse.readFileSync(fileName);
            const mimeType = mime.lookup(fileName)
            
            if (!mimeType) {
              throw "unknown icon file type, deleting...";
            }
           
            cluster.preferences.icon = `data:${mimeType};base64, ${fileData.toString('base64')}`;
...

and that seem to work better.

nevalla · 2020-08-19T11:32:33Z

oh, noticed that images seem to be rendered fine without mimetype too, ie data:;base64, ..... But should not hurt to put correct in place.

nevalla

Please take a look the comment I've added.

Nokel81 · 2020-08-19T19:05:49Z

@nevalla mime-types is not the same as as file-type. The former merely looks at the file extension while the later use the actual contents of the file.

nevalla · 2020-08-19T19:27:42Z

@nevalla mime-types is not the same as as file-type. The former merely looks at the file extension while the later use the actual contents of the file.

Okay, I see. Nevertheless, I had some issues with this line:

const { mime } = makeSynchronous(() => fileType.fromBuffer(fileData))();

I did not see any errors in the logs but for some weird reason the default electron window opened.

Nokel81 · 2020-08-19T19:30:19Z

fair, let me take a look

Nokel81 · 2020-08-20T13:01:17Z

@nevalla changed to base master

ixrock · 2020-08-20T14:30:10Z

package.json

@@ -184,6 +185,7 @@
    "jsonpath": "^1.0.2",
    "lodash": "^4.17.15",
    "mac-ca": "^1.0.4",
+    "make-synchronous": "^0.1.0",


Why do we need this?

Because file-type is async only but migrations have to be sync only

ixrock · 2020-08-21T11:18:20Z

package.json

@@ -184,6 +185,7 @@
    "jsonpath": "^1.0.2",
    "lodash": "^4.17.15",
    "mac-ca": "^1.0.4",
+    "make-synchronous": "https://github.com/Nokel81/make-synchronous#e366285001da13f4d5c028888a56b9baf83f9cff",


Why not from npm? Btw maybe we could support async migration and avoid this to overcomplicating simple things?

This was temporary because I had made some changes that had not yet been merged upstream to this package. A comparable fix has now been published, so I will change back.

We cannot support async migration because the library that we use doesn't support it at all

ixrock · 2020-08-21T11:22:13Z

src/migrations/cluster-store/3.6.0-beta.1.ts

+const AsyncFunction = Object.getPrototypeOf(async function () { return }).constructor;
+const getFileTypeFnString = `return require("file-type").fromBuffer(fileData)`;
+const getFileType = new AsyncFunction("fileData", getFileTypeFnString);


This looks crazy to my eyes.. What have you tried to achieve with this? Why evaluating JS-string?

Fair, I am just going to say fair since this was annoying to get right.

This is needed for several things:

webpack is used which mucks with require and await import so we need someway to deal with that. There is __non_webpack_require__ but that only exists when webpack is being used (which it is not during tests)

It cannot be just a normal eval since the type that make-synchronous uses is AsyncFunction

AsyncFunction is not a global and I got that first line from mozilla's web docs.

ixrock · 2020-08-21T11:24:32Z

src/migrations/cluster-store/3.6.0-beta.1.ts

-    const migratedClusters: ClusterModel[] = []
-    const storedClusters: ClusterModel[] = store.get("clusters");
-    const kubeConfigBase = path.join((app || remote.app).getPath("userData"), "kubeconfigs")
+    const userDataPath = (app || remote.app).getPath("userData")


I'm wondering is it even possible to stop using remote.app in runtime at some point and still support it in migrations?

Dunno, I just wanted to call this once, since it is used in many places.

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

…-type package Signed-off-by: Sebastian Malton <smalton@mirantis.com>

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

nevalla

Migration seems to work now correctly

ixrock · 2020-08-27T13:00:19Z

src/common/cluster-store_test.ts

            preferences: {
              terminalCWD: "/tmp"
            }
          },
-        })
+        }),
+        "icon_path": testDataIcon,


How does it work? testDataIcon should be a Buffer type?

ixrock · 2020-08-27T13:00:42Z

src/common/cluster-store_test.ts

@@ -6,6 +6,10 @@ import { ClusterStore } from "./cluster-store";
 import { workspaceStore } from "./workspace-store";
 import { saveConfigToAppFiles } from "./kube-helpers";

+const testDataIcon = fs.readFileSync("test-data/cluster-store-migration-icon.png")


Can we remove it or at least move to __mocks__ or src/migrations folder?

Nokel81 requested a review from ixrock August 11, 2020 15:00

Nokel81 self-assigned this Aug 11, 2020

Nokel81 requested a review from jakolehm August 11, 2020 15:00

ixrock approved these changes Aug 11, 2020

View reviewed changes

Nokel81 requested a review from ixrock August 11, 2020 16:55

nevalla requested changes Aug 19, 2020

View reviewed changes

nevalla closed this Aug 20, 2020

nevalla reopened this Aug 20, 2020

Nokel81 force-pushed the icon-migration branch from 6e1c37e to cdc4d96 Compare August 20, 2020 13:00

Nokel81 changed the base branch from vue_react_migration to master August 20, 2020 13:01

ixrock reviewed Aug 20, 2020

View reviewed changes

ixrock reviewed Aug 21, 2020

View reviewed changes

Nokel81 requested a review from nevalla August 21, 2020 14:38

Sebastian Malton added 7 commits August 21, 2020 10:39

add cluster icon migration code

0869bb9

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

add MIME type detection in the migration

37d9681

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

revert back to syncronous migrations. Use make-syncronous to use file…

e5a7f9f

…-type package Signed-off-by: Sebastian Malton <smalton@mirantis.com>

removing missed async

09f2f5e

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

remove icons dir since no longer needed

bc9476e

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

correctly handle the previous icon fields value

35b42de

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

fix migration not working, add unit tests

4eea30a

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

move to new version of make-synchronous

7629e69

Signed-off-by: Sebastian Malton <smalton@mirantis.com>

Nokel81 force-pushed the icon-migration branch from 7cc0b2c to 7629e69 Compare August 21, 2020 14:39

nevalla approved these changes Aug 24, 2020

View reviewed changes

nevalla added this to the 3.6.0 milestone Aug 25, 2020

nevalla added the bug Something isn't working label Aug 25, 2020

nevalla merged commit a12aa8a into lensapp:master Aug 25, 2020

ixrock reviewed Aug 27, 2020

View reviewed changes

nevalla mentioned this pull request Aug 28, 2020

Release v3.6.0-beta.1 #762

Merged

nevalla mentioned this pull request Sep 14, 2020

Release v3.6.0 #870

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add cluster icon migration code #673

add cluster icon migration code #673

Nokel81 commented Aug 11, 2020

ixrock left a comment

ixrock Aug 11, 2020

Nokel81 Aug 11, 2020

Nokel81 commented Aug 11, 2020

nevalla commented Aug 17, 2020

nevalla commented Aug 19, 2020 •

edited

nevalla commented Aug 19, 2020

nevalla left a comment

Nokel81 commented Aug 19, 2020

nevalla commented Aug 19, 2020

Nokel81 commented Aug 19, 2020

Nokel81 commented Aug 20, 2020

ixrock Aug 20, 2020

Nokel81 Aug 20, 2020

ixrock Aug 21, 2020

Nokel81 Aug 21, 2020

Nokel81 Aug 21, 2020

ixrock Aug 21, 2020

Nokel81 Aug 21, 2020

Nokel81 Aug 21, 2020

ixrock Aug 21, 2020

Nokel81 Aug 21, 2020

nevalla left a comment

ixrock Aug 27, 2020

ixrock Aug 27, 2020 •

edited

add cluster icon migration code #673

add cluster icon migration code #673

Conversation

Nokel81 commented Aug 11, 2020

ixrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nokel81 commented Aug 11, 2020

nevalla commented Aug 17, 2020

nevalla commented Aug 19, 2020 • edited

nevalla commented Aug 19, 2020

nevalla left a comment

Choose a reason for hiding this comment

Nokel81 commented Aug 19, 2020

nevalla commented Aug 19, 2020

Nokel81 commented Aug 19, 2020

Nokel81 commented Aug 20, 2020

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

nevalla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixrock Aug 27, 2020 • edited

Choose a reason for hiding this comment

nevalla commented Aug 19, 2020 •

edited

ixrock Aug 27, 2020 •

edited