Add lens:// protocol handling with a routing mechanism #1949

Nokel81 · 2021-01-12T22:39:27Z

Things that still need to be done:

Testing on linux
Testing on windows
Add markdown docs

Signed-off-by: Sebastian Malton [email protected]

fixes #1288

jakolehm

Looks pretty good already 👍 Could we add integration tests for protocol handler?

src/extensions/lens-main-extension.ts

Nokel81 · 2021-01-14T13:28:56Z

I will write some integration tests.

stefcameron · 2021-01-15T19:36:04Z

Great to see this going in! @Nokel81 are there any changes from the original implementation?

Nokel81 · 2021-01-15T19:39:55Z

Great to see this going in! @Nokel81 are there any changes from the original implementation?

From a users perspective no. So your extension technically doesn't have to be even updated. However, from an implementation standpoint. This now supports handlers from both renderer and main. Our hackweek version only supported renderer handlers.

stefcameron · 2021-01-15T19:40:50Z

Great to see this going in! @Nokel81 are there any changes from the original implementation?

From a users perspective no. So your extension technically doesn't have to be even updated. However, from an implementation standpoint. This now supports handlers from both renderer and main. Our hackweek version only supported renderer handlers.

That's cool! I remember you wanted to add support to main as well but didn't have time then. As far as the API goes, though, no changes?

Nokel81 · 2021-01-15T19:42:06Z

Great to see this going in! @Nokel81 are there any changes from the original implementation?

From a users perspective no. So your extension technically doesn't have to be even updated. However, from an implementation standpoint. This now supports handlers from both renderer and main. Our hackweek version only supported renderer handlers.

That's cool! I remember you wanted to add support to main as well but didn't have time then. As far as the API goes, though, no changes?

None, the method on LensRendererExtension (and LensMainExtension for that matter) is still onProtocolRequest.

Nokel81 · 2021-01-15T19:42:26Z

I even checked what your extension was expecting to make sure I didn't change anything 😉

stefcameron · 2021-01-15T19:44:08Z

I even checked what your extension was expecting to make sure I didn't change anything ;)

Fantastic! Thanks for testing it out. 😄

docs/extensions/guides/protocol-handlers.md

src/common/protocol-handler/router.ts

src/extensions/lens-main-extension.ts

src/main/protocol-handler/__test__/router.test.ts

Nokel81 · 2021-01-15T21:31:48Z

@nevalla any ideas about why the integration tests for mac and linux are not being reported as failing in the CI, even though if you look at the logs there are failed tests?

src/common/protocol-handler/router.ts

jim-docker · 2021-01-15T22:16:17Z

@nevalla any ideas about why the integration tests for mac and linux are not being reported as failing in the CI, even though if you look at the logs there are failed tests?

For this PR the only failed test I see for windows and mac is the protocol app start test. There's no minikube set up on Windows or mac so those tests are skipped. Or do you see other failures for other builds?

jim-docker · 2021-01-15T23:37:35Z

@nevalla any ideas about why the integration tests for mac and linux are not being reported as failing in the CI, even though if you look at the logs there are failed tests?

For this PR the only failed test I see for windows and mac is the protocol app start test. There's no minikube set up on Windows or mac so those tests are skipped. Or do you see other failures for other builds?

Oh, but the CI reports success. Ruh Roh.

nevalla · 2021-01-18T10:13:05Z

@nevalla any ideas about why the integration tests for mac and linux are not being reported as failing in the CI, even though if you look at the logs there are failed tests?

Yes those should have been reported as failed. Hard to say why it's not the case here, need to investigate further.

nevalla · 2021-01-18T10:21:44Z

Okay...I think I got it. We are running git checkout as the last step and exit code of that command is returned:

 - bash: |
      rm -rf extensions/telemetry
      make integration-win
      git checkout extensions/telemetry
    displayName: Run integration tests

We might want to return the exit code from make integration-* command.

Nokel81 · 2021-01-18T13:28:09Z

Okay...I think I got it. We are running git checkout as the last step and exit code of that command is returned:
 - bash: |
      rm -rf extensions/telemetry
      make integration-win
      git checkout extensions/telemetry
    displayName: Run integration tests
We might want to return the exit code from make integration-* command.

Thanks will fix that.

Nokel81 · 2021-01-18T13:42:56Z

@stefcameron I didn't test your extension. I only made sure that the API was compatible.

stefcameron · 2021-01-18T16:56:11Z

@stefcameron I didn't test your extension. I only made sure that the API was compatible.

Oh, thanks for clarifying that. I'll git it a try against this branch then to make sure it still works.

jim-docker · 2021-01-18T17:37:19Z

I think the protocol name shoud be something less generic than lens, like k8slens, since it is global (lens protocol could already be in use somewhere?)

Nokel81 · 2021-01-18T18:15:24Z

I think the protocol name shoud be something less generic than lens, like k8slens, since it is global (lens protocol could already be in use somewhere?)

As seen on slack, it has been decided to keep it as lens

Signed-off-by: Sebastian Malton <[email protected]>

…l-handler Signed-off-by: Sebastian Malton <[email protected]>

…n tests Signed-off-by: Sebastian Malton <[email protected]>

Signed-off-by: Sebastian Malton <[email protected]>

…l-handler Signed-off-by: Sebastian Malton <[email protected]>

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker · 2021-02-18T19:00:27Z

src/main/app-updater.ts

@@ -26,7 +27,7 @@ function handleAutoUpdateBackChannel(event: Electron.IpcMainEvent, ...[arg]: Upd
 * starts the automatic update checking
 * @param interval milliseconds between interval to check on, defaults to 24h
 */
-export function startUpdateChecking(interval = 1000 * 60 * 60 * 24): void {
+export const startUpdateChecking = once(function (interval = 1000 * 60 * 60 * 24): void {


Does this fix an unrelated issue? Or just refactoring while accommodating protocol handling?

It was fixing an issue due to the changes related to src/main/index.ts:158. Namely, if "renderer:loaded" gets called multiple times startUpdateChecking shouldn't be called on the subsequent events.

I figured, but is it still a bug otherwise? If so, shouldn't it really be in its own PR?

I figured that I should fix it in this PR since it was in this PR that I changed how the renderer lets main know it has been loaded.

jim-docker · 2021-02-18T19:00:51Z

src/main/index.ts

@@ -37,30 +38,54 @@ let windowManager: WindowManager;

 app.setName(appName);

+logger.info("📟 Setting as Lens as protocol client for lens://");


Suggested change

logger.info("📟 Setting as Lens as protocol client for lens://");

logger.info("📟 Setting Lens as protocol client for lens://");

Good catch, though I will do that in a follow up PR so as to not need new approves.

stefcameron · 2021-02-18T23:02:01Z

🎉 It's finally GREEN!!! 💚

jakolehm

LGTM. Let's keep improving this in follow-up PRs.

Nokel81 added enhancement New feature or request area/windows area/linux area/macos area/documentation labels Jan 12, 2021

Nokel81 requested a review from a team January 12, 2021 22:39

Nokel81 self-assigned this Jan 12, 2021

jakolehm reviewed Jan 14, 2021

View reviewed changes

src/extensions/lens-main-extension.ts Outdated Show resolved Hide resolved

Nokel81 force-pushed the feature/protocol-handler branch from e601ffa to f663b82 Compare January 14, 2021 18:06

jim-docker reviewed Jan 15, 2021

View reviewed changes

stefcameron reviewed Jan 15, 2021

View reviewed changes

src/common/protocol-handler/router.ts Outdated Show resolved Hide resolved

stefcameron reviewed Jan 15, 2021

View reviewed changes

src/common/protocol-handler/router.ts Outdated Show resolved Hide resolved

lensapp deleted a comment Jan 18, 2021

Nokel81 added 2 commits February 4, 2021 08:57

remove killall from integration tests

eb4c98e

Signed-off-by: Sebastian Malton <[email protected]>

fix bad merge

1c8aa2b

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker previously approved these changes Feb 4, 2021

View reviewed changes

create dir if necessary (for testing)

c8faded

Signed-off-by: Sebastian Malton <[email protected]>

Nokel81 dismissed jim-docker’s stale review via c8faded February 4, 2021 17:16

Nokel81 added 2 commits February 4, 2021 14:12

remove linux based protocol handling setup for testing

bcbb335

Signed-off-by: Sebastian Malton <[email protected]>

Merge branch 'master' of github.com:lensapp/lens into feature/protoco…

96e3c52

…l-handler Signed-off-by: Sebastian Malton <[email protected]>

jakolehm previously approved these changes Feb 8, 2021

View reviewed changes

jim-docker previously approved these changes Feb 8, 2021

View reviewed changes

Merge branch 'master' of github.com:lensapp/lens into feature/protoco…

42928a8

…l-handler Signed-off-by: Sebastian Malton <[email protected]>

Nokel81 dismissed stale reviews from jim-docker and jakolehm via 42928a8 February 9, 2021 16:09

Sebastian Malton and others added 6 commits February 9, 2021 11:12

remove unneccessary shorter timeouts on before* handles in integratio…

69ce254

…n tests Signed-off-by: Sebastian Malton <[email protected]>

move remove config to make targets

b48b38c

Signed-off-by: Sebastian Malton <[email protected]>

fix lint

74fc8b4

Signed-off-by: Sebastian Malton <[email protected]>

fail tests in before and after handles

7395d0c

Signed-off-by: Sebastian Malton <[email protected]>

switch to event emitting renderer being ready

311178f

Signed-off-by: Sebastian Malton <[email protected]>

Add logging and fix renderer:loaded send to main

270d0f7

Signed-off-by: Sebastian Malton <[email protected]>

Nokel81 force-pushed the feature/protocol-handler branch from a77c07b to 270d0f7 Compare February 10, 2021 18:51

Nokel81 added 3 commits February 18, 2021 10:04

Merge branch 'master' of github.com:lensapp/lens into feature/protoco…

65274f0

…l-handler Signed-off-by: Sebastian Malton <[email protected]>

revert integration test code

c80965e

Signed-off-by: Sebastian Malton <[email protected]>

ignore removeing old config

5ee659d

Signed-off-by: Sebastian Malton <[email protected]>

jim-docker approved these changes Feb 18, 2021

View reviewed changes

jakolehm approved these changes Feb 24, 2021

View reviewed changes

Nokel81 merged commit 1470103 into master Feb 25, 2021

Nokel81 deleted the feature/protocol-handler branch February 25, 2021 14:32

Nokel81 added this to the 4.2.0 milestone Mar 5, 2021

This was referenced Mar 5, 2021

Release 4.2.0-alpha.0 #2283

Closed

Release 4.2.0-alpha.1 #2285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lens:// protocol handling with a routing mechanism #1949

Add lens:// protocol handling with a routing mechanism #1949

Nokel81 commented Jan 12, 2021 •

edited

Loading

jakolehm left a comment

Nokel81 commented Jan 14, 2021

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

Nokel81 commented Jan 15, 2021 •

edited

Loading

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

jim-docker commented Jan 15, 2021

jim-docker commented Jan 15, 2021

nevalla commented Jan 18, 2021

nevalla commented Jan 18, 2021 •

edited

Loading

Nokel81 commented Jan 18, 2021

Nokel81 commented Jan 18, 2021

stefcameron commented Jan 18, 2021

jim-docker commented Jan 18, 2021 •

edited

Loading

Nokel81 commented Jan 18, 2021

jim-docker Feb 18, 2021

Nokel81 Feb 18, 2021

jim-docker Feb 18, 2021

Nokel81 Feb 18, 2021

jim-docker Feb 18, 2021

Nokel81 Feb 18, 2021

stefcameron commented Feb 18, 2021

jakolehm left a comment

		@@ -37,30 +38,54 @@ let windowManager: WindowManager;

		app.setName(appName);

		logger.info("📟 Setting as Lens as protocol client for lens://");

	logger.info("📟 Setting as Lens as protocol client for lens://");
	logger.info("📟 Setting Lens as protocol client for lens://");

Add lens:// protocol handling with a routing mechanism #1949

Add lens:// protocol handling with a routing mechanism #1949

Conversation

Nokel81 commented Jan 12, 2021 • edited Loading

jakolehm left a comment

Choose a reason for hiding this comment

Nokel81 commented Jan 14, 2021

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

Nokel81 commented Jan 15, 2021 • edited Loading

stefcameron commented Jan 15, 2021

Nokel81 commented Jan 15, 2021

jim-docker commented Jan 15, 2021

jim-docker commented Jan 15, 2021

nevalla commented Jan 18, 2021

nevalla commented Jan 18, 2021 • edited Loading

Nokel81 commented Jan 18, 2021

Nokel81 commented Jan 18, 2021

stefcameron commented Jan 18, 2021

jim-docker commented Jan 18, 2021 • edited Loading

Nokel81 commented Jan 18, 2021

jim-docker Feb 18, 2021

Choose a reason for hiding this comment

Nokel81 Feb 18, 2021

Choose a reason for hiding this comment

jim-docker Feb 18, 2021

Choose a reason for hiding this comment

Nokel81 Feb 18, 2021

Choose a reason for hiding this comment

jim-docker Feb 18, 2021

Choose a reason for hiding this comment

Nokel81 Feb 18, 2021

Choose a reason for hiding this comment

stefcameron commented Feb 18, 2021

jakolehm left a comment

Choose a reason for hiding this comment

Nokel81 commented Jan 12, 2021 •

edited

Loading

Nokel81 commented Jan 15, 2021 •

edited

Loading

nevalla commented Jan 18, 2021 •

edited

Loading

jim-docker commented Jan 18, 2021 •

edited

Loading