Make routes in back-end comply to Open Closed Principle #4859

jansav · 2022-02-14T10:15:14Z

Motivation:

I should be able to add route with minimal modifications (none)
Route should have it's own dependencies
Route shouldn't be responsible of e.g. error handling which can be abstracted to higher level.

Solution:

Introduce routeInjectionToken which any back-end route can implement by adding injectionToken: routeInjectionToken. Any injectable implementing said injectionToken will be registered automatically.

Note: base-branch is PR #4842 so let's get that merged first.

TBD later:

Get rid of IncomingMessage and http.ServerResponse interface from router so that new routes can be unit tested easily even when using router in tests.

github-actions · 2022-02-14T11:30:48Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-02-14T11:37:13Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

ixrock

Didn't run the app, but code LGTM.

ixrock · 2022-02-23T13:57:49Z

src/main/router.ts

-import logger from "./logger";
+import type { LensApiResultContentType } from "./router-content-types";
+import { contentTypes } from "./router-content-types";
+import "../common/vars";


Why do we need import whole package without using anything locally from it?

My question exactly... There is side-effect in common/vars which defines __static and therefore is required. Something that is of course wrong, but out of scope in this PR.

Maybe add comment/todo so that next reader knows why it's there?

src/main/routes/static-file-route.injectable.ts

ixrock · 2022-02-23T14:16:34Z

package.json

@@ -326,6 +326,7 @@
    "@typescript-eslint/eslint-plugin": "^5.10.1",
    "@typescript-eslint/parser": "^5.10.1",
    "ansi_up": "^5.1.0",
+    "mock-http": "^1.1.0",


Why do we need this package / extra dependency? Can we live without it?

At the moment, no.

Get rid of IncomingMessage and http.ServerResponse interface from router so that new routes can be unit tested easily even when using router in tests.

However, we can after this.

chenhunghan · 2022-02-25T11:40:05Z

src/main/helm/helm-release-manager.ts

@@ -108,7 +108,7 @@ export async function upgradeRelease(name: string, chart: string, values: any, n

    return {
      log: output,
-      release: getRelease(name, namespace, kubeconfigPath, kubectlPath),
+      release: await getRelease(name, namespace, kubeconfigPath, kubectlPath),


Noticed no catch in this try...finally, will await getRelease throw? Or error is caught and handled somewhere else?

Key difference here is that all back-end API responds go through same place which already catches fatal errors. That being said, if it fails fatally, API call is resolved with status code 422 and error message. And in this case, there is no non-fatal error to be handled. Goal is to get rid of try-catches in operational code for good (strategic places that handle errors for everything to allow error monitoring etc). Throwing is mechanism that is reserved for fatal errors which should prevent code to be executed and for non fatal errors we have better alternatives.

github-actions · 2022-02-25T16:12:35Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-02-28T06:04:54Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2022-02-28T14:39:48Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-03-03T10:44:08Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

src/main/router-content-types.ts

src/main/router.test.ts

Nokel81 · 2022-03-03T13:20:45Z

src/main/router.ts

 interface Dependencies {
-  routePortForward: (request: LensApiRequest) => Promise<void>;
+  parseRequest: (request: http.IncomingMessage, _: null, options: { parse: boolean; output: string }) => Promise<{ payload: any }>;


why even have the second param?

It's third-party library.

Yeah but we can wrap it if we want

src/main/router.ts

Nokel81 · 2022-03-03T13:35:01Z

src/main/router.ts

+  writeServerResponse(mappedResult);
+};
+
+const writeServerResponseFor =


nit, all this extra whitespace makes it harder to read. Could this be a normal function or at least add some ()?

Nokel81 · 2022-03-03T13:36:41Z

src/main/routes/helm/charts/get-chart-route.injectable.ts

+
+  instantiate: (): Route<GetChartResponse> => ({
+    method: "get",
+    path: `${apiPrefix}/v2/charts/{repo}/{chart}`,


I have (what I think) is a pretty cool idea of how to make sure that this string is "correctly typed" with regards to the interface that Params expects below but we can do that later.

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

…t interesting Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

github-actions · 2022-03-11T07:52:22Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

jakolehm

LGTM

jansav added the chore label Feb 14, 2022

jansav added this to the 5.5.0 milestone Feb 14, 2022

jansav requested a review from a team as a code owner February 14, 2022 10:15

jansav requested review from jweak and Iku-turso and removed request for a team February 14, 2022 10:15

jansav force-pushed the update-injectable branch from 265654c to eecd4d5 Compare February 14, 2022 11:30

github-actions bot added the PR: needs rebase label Feb 14, 2022

jansav force-pushed the back-end-routes branch from cf53336 to 5c8ae0b Compare February 14, 2022 11:37

github-actions bot removed the PR: needs rebase label Feb 14, 2022

jansav force-pushed the back-end-routes branch from 5c8ae0b to f23e559 Compare February 14, 2022 11:41

jansav mentioned this pull request Feb 14, 2022

Stop using HelmCli from Renderer #4861

Merged

ixrock previously approved these changes Feb 23, 2022

View reviewed changes

chenhunghan reviewed Feb 25, 2022

View reviewed changes

Base automatically changed from update-injectable to master February 25, 2022 16:10

github-actions bot added the PR: needs rebase label Feb 25, 2022

jansav dismissed ixrock’s stale review via ae0ecc5 February 28, 2022 06:04

jansav force-pushed the back-end-routes branch from 95d9152 to ae0ecc5 Compare February 28, 2022 06:04

github-actions bot removed the PR: needs rebase label Feb 28, 2022

github-actions bot added the PR: needs rebase label Feb 28, 2022

jansav force-pushed the back-end-routes branch from 277afb4 to 3989939 Compare March 3, 2022 10:43

github-actions bot removed the PR: needs rebase label Mar 3, 2022

Iku-turso previously approved these changes Mar 3, 2022

View reviewed changes

Nokel81 reviewed Mar 3, 2022

View reviewed changes

jansav added 22 commits March 11, 2022 08:55

Switch to using injectable to setup route for metrics

ef92a98

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Switch to using injectable to setup route for getting service account

c41d10d

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Switch to using injectable to setup route for getting version

3ab356b

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Add global override for reading file to make it not required where no…

086db9f

…t interesting Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Switch to using injectable to setup route for serving static files

711516c

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make adding routes private for router

afa9c10

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make routes wait until all asynchronous stuff are done

d099256

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Introduce healthy abstraction between back-end routes and route handlers

4b76327

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make response of route typed

7958751

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix incorrect return value of updateRelease

3f28a09

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make typing of routes support synchronous values

9117bc2

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make code cleaner

adde132

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Rename test for accuracy

878488f

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Add test for throwing route handler

b571781

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Remove duplicate license header

40bf8e7

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Make mechanism of creating HTTP response an implementation detail

db58733

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Remove not needed properties from test

4e09ebd

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix code style

00aef47

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix typing and codestyle

e5f53e1

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix typing error

514e546

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Fix merge conflicts

7e511b5

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

Organize all router related files under directory

6804c6e

Signed-off-by: Janne Savolainen <janne.savolainen@live.fi>

jansav force-pushed the back-end-routes branch from 6df314a to 6804c6e Compare March 11, 2022 07:52

github-actions bot removed the PR: needs rebase label Mar 11, 2022

jakolehm approved these changes Mar 11, 2022

View reviewed changes

Iku-turso approved these changes Mar 11, 2022

View reviewed changes

jansav merged commit 38af26e into master Mar 11, 2022

jansav deleted the back-end-routes branch March 11, 2022 10:20

Nokel81 mentioned this pull request May 4, 2022

release v5.5.0-beta.0 #5326

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make routes in back-end comply to Open Closed Principle #4859

Make routes in back-end comply to Open Closed Principle #4859

jansav commented Feb 14, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022

github-actions bot commented Feb 14, 2022

ixrock left a comment

ixrock Feb 23, 2022

jansav Feb 25, 2022 •

edited

Loading

jakolehm Mar 7, 2022

ixrock Feb 23, 2022

jansav Feb 25, 2022

chenhunghan Feb 25, 2022

jansav Feb 28, 2022

github-actions bot commented Feb 25, 2022

github-actions bot commented Feb 28, 2022

github-actions bot commented Feb 28, 2022

github-actions bot commented Mar 3, 2022

Nokel81 Mar 3, 2022

jansav Mar 3, 2022

Nokel81 Mar 3, 2022

Nokel81 Mar 3, 2022

Nokel81 Mar 3, 2022

github-actions bot commented Mar 11, 2022

jakolehm left a comment

Make routes in back-end comply to Open Closed Principle #4859

Make routes in back-end comply to Open Closed Principle #4859

Conversation

jansav commented Feb 14, 2022 • edited Loading

Motivation:

Solution:

TBD later:

github-actions bot commented Feb 14, 2022

github-actions bot commented Feb 14, 2022

ixrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jansav Feb 25, 2022 • 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

github-actions bot commented Feb 25, 2022

github-actions bot commented Feb 28, 2022

github-actions bot commented Feb 28, 2022

github-actions bot commented Mar 3, 2022

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

github-actions bot commented Mar 11, 2022

jakolehm left a comment

Choose a reason for hiding this comment

jansav commented Feb 14, 2022 •

edited

Loading

jansav Feb 25, 2022 •

edited

Loading