-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Fix failing to start shell being hidden from user #5626
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 20 commits
ed6f522
87dc14c
dba4f59
8136e23
f111450
c9f1594
2de3068
d293554
988307e
071ba8a
ea603c7
78b05ba
aa41117
19466f1
d1acca4
f2d19e7
2a96eb8
862cd2a
3c9791e
c340256
f522d23
37955f0
b86bc6d
4c1d076
c6a9d56
e49b0f6
d173e45
b96e115
6ce833c
0dabbb6
a82a120
e90a181
9c75199
97dfe4d
e8bc887
58cbf5f
8761aa1
8fd732b
e9e248e
71ad8e1
3a7ac10
c11d8c6
0088add
6321b10
9683a7c
a271c83
3b72137
19a8691
f6b5214
78614af
56703cd
a1d2fa0
3fbbf02
8b18e58
d9a7a48
ea49929
0e1b2c0
12bd506
2f43d24
14f50a3
6d0e527
d22af76
3492741
6dd926a
cfad2c0
186d7f9
9adddaf
e88743d
4885461
120a5f7
91cd2c8
7971755
c658a0b
9b7e77d
4815510
3471397
e77411a
d1eb149
6880d52
d2ee54f
8704067
9539c1c
5f30b81
4edfa40
29a7e2a
51b8c91
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole file feels like an amazing kludge, and brittle too. Is there really no other way?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately I could not find one after banging my head for a few days. The fact that we use Electron seem to break some of the dependencies auto detecting certain configuration things. |
||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import child_process from "child_process"; | ||
| import fsExtra from "fs-extra"; | ||
| import fetch from "node-fetch"; | ||
| import { platform } from "process"; | ||
| import stream from "stream"; | ||
| import { promisify } from "util"; | ||
| import { extract } from "tar"; | ||
|
|
||
| const { ensureDir, readJson } = fsExtra; | ||
|
|
||
| function canvasPrebuiltUrlBuilder(canvasVersion: string, nodeVersion: string) { | ||
| const compiler = platform === "linux" | ||
| ? "glibc" | ||
| : "unknown"; | ||
|
|
||
| return `https://github.com/Automattic/node-canvas/releases/download/v${canvasVersion}/canvas-v${canvasVersion}-node-v${nodeVersion}-${platform}-${compiler}-x64.tar.gz`; | ||
| } | ||
|
|
||
| const exec = promisify(child_process.exec); | ||
| const pipeline = promisify(stream.pipeline); | ||
|
|
||
| // This is done so that we can skip the scripts for only this package | ||
| await exec("npm install canvas@2 --no-save --no-package-lock --ignore-scripts --legacy-peer-deps"); | ||
|
|
||
| const nodeModuleVersion = process.versions.modules; | ||
| const canvasVersion = (await readJson("./node_modules/canvas/package.json")).version as string; | ||
| const canvasPrebuildUrl = canvasPrebuiltUrlBuilder(canvasVersion, nodeModuleVersion); | ||
|
|
||
| const canvasPrebuilt = await fetch(canvasPrebuildUrl); | ||
|
|
||
| if (canvasPrebuilt.status !== 200) { | ||
| throw new Error(`Failed to download prebuilt from ${canvasPrebuildUrl}: ${canvasPrebuilt.statusText}`); | ||
| } | ||
|
|
||
| await ensureDir("./node_modules/canvas/build"); | ||
|
|
||
| const dest = extract({ | ||
| cwd: "./node_modules/canvas/build", | ||
| }); | ||
|
|
||
| await pipeline(canvasPrebuilt.body, dest); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -56,6 +56,10 @@ describe("app-paths", () => { | |
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| builder.quit(); | ||
| }); | ||
|
|
||
|
||
| describe("normally", () => { | ||
| let windowDi: DiContainer; | ||
| let mainDi: DiContainer; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,12 +20,12 @@ import { disposer, isDefined, isRequestError, toJS } from "../utils"; | |
| import type { Response } from "request"; | ||
|
||
| import { clusterListNamespaceForbiddenChannel } from "../ipc/cluster"; | ||
| import type { CanI } from "./authorization-review.injectable"; | ||
| import type { ListNamespaces } from "./list-namespaces.injectable"; | ||
| import type { ListNamespacesFor } from "./list-namespaces.injectable"; | ||
| import assert from "assert"; | ||
| import type { Logger } from "../logger"; | ||
| import type { BroadcastMessage } from "../ipc/broadcast-message.injectable"; | ||
| import type { LoadConfigfromFile } from "../kube-helpers/load-config-from-file.injectable"; | ||
| import type { CanListResource, RequestNamespaceListPermissions, RequestNamespaceListPermissionsFor } from "./request-namespace-list-permissions.injectable"; | ||
| import type { RequestNamespaceListPermissions, RequestNamespaceListPermissionsFor } from "./request-namespace-list-permissions.injectable"; | ||
| import type { RequestApiResources } from "../../main/cluster/request-api-resources.injectable"; | ||
|
|
||
| export interface ClusterDependencies { | ||
|
|
@@ -38,7 +38,7 @@ export interface ClusterDependencies { | |
| createAuthorizationReview: (config: KubeConfig) => CanI; | ||
| requestApiResources: RequestApiResources; | ||
| requestNamespaceListPermissionsFor: RequestNamespaceListPermissionsFor; | ||
| createListNamespaces: (config: KubeConfig) => ListNamespaces; | ||
| listNamespacesFor: ListNamespacesFor; | ||
| createVersionDetector: (cluster: Cluster) => VersionDetector; | ||
| broadcastMessage: BroadcastMessage; | ||
| loadConfigfromFile: LoadConfigfromFile; | ||
|
|
@@ -644,7 +644,7 @@ export class Cluster implements ClusterModel { | |
| } | ||
|
|
||
| try { | ||
| const listNamespaces = this.dependencies.createListNamespaces(proxyConfig); | ||
| const listNamespaces = this.dependencies.listNamespacesFor(proxyConfig); | ||
|
|
||
| return await listNamespaces(); | ||
| } catch (error) { | ||
|
|
@@ -672,10 +672,9 @@ export class Cluster implements ClusterModel { | |
| const canListResourceCheckers = await Promise.all(( | ||
| this.allowedNamespaces.map(namespace => apiLimit(() => requestNamespaceListPermissions(namespace))) | ||
| )); | ||
| const canListNamespacedResource: CanListResource = (resource) => canListResourceCheckers.some(fn => fn(resource)); | ||
|
|
||
| return this.knownResources | ||
| .filter(canListNamespacedResource) | ||
| .filter((resource) => canListResourceCheckers.some(fn => fn(resource))) | ||
| .map(formatKubeApiResource); | ||
| } catch (error) { | ||
| return []; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getGlobalOverride } from "../test-utils/get-global-override"; | ||
| import changePathModeInjectable from "./change-path-mode.injectable"; | ||
|
|
||
| export default getGlobalOverride(changePathModeInjectable, () => () => { | ||
| throw new Error("tried to change path mode without override"); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getInjectable } from "@ogre-tools/injectable"; | ||
| import fsInjectable from "./fs.injectable"; | ||
|
|
||
| export type ChangePathMode = (path: string, newMode: number) => Promise<void>; | ||
|
|
||
| const changePathModeInjectable = getInjectable({ | ||
| id: "change-path-mode", | ||
| instantiate: (di): ChangePathMode => di.inject(fsInjectable).chmod, | ||
| }); | ||
|
|
||
| export default changePathModeInjectable; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getGlobalOverride } from "../test-utils/get-global-override"; | ||
| import copyFileInjectable from "./copy-file.injectable"; | ||
|
|
||
| export default getGlobalOverride(copyFileInjectable, () => () => { | ||
| throw new Error("tried to copy a file without override"); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
| import { getInjectable } from "@ogre-tools/injectable"; | ||
| import fsInjectable from "./fs.injectable"; | ||
|
|
||
| export type CopyFile = (fromPath: string, toPath: string) => Promise<void>; | ||
|
|
||
| const copyFileInjectable = getInjectable({ | ||
| id: "copy-file", | ||
| instantiate: (di): CopyFile => di.inject(fsInjectable).copyFile, | ||
| }); | ||
|
|
||
| export default copyFileInjectable; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getGlobalOverride } from "../test-utils/get-global-override"; | ||
| import createReadFileStreamInjectable from "./create-read-file-stream.injectable"; | ||
|
|
||
| export default getGlobalOverride(createReadFileStreamInjectable, () => () => { | ||
| throw new Error("tried to create read stream for a file without override"); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getInjectable } from "@ogre-tools/injectable"; | ||
| import type { createWriteStream } from "fs"; | ||
| import fsInjectable from "./fs.injectable"; | ||
|
|
||
| export type CreateWriteFileStream = typeof createWriteStream; | ||
|
|
||
| const createWriteFileStreamInjectable = getInjectable({ | ||
| id: "create-write-file-stream", | ||
| instantiate: (di) => di.inject(fsInjectable).createWriteStream, | ||
| }); | ||
|
|
||
| export default createWriteFileStreamInjectable; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getGlobalOverride } from "../test-utils/get-global-override"; | ||
| import createWriteFileStreamInjectable from "./create-write-file-stream.injectable"; | ||
|
|
||
| export default getGlobalOverride(createWriteFileStreamInjectable, () => () => { | ||
| throw new Error("tried to create a file write stream without override"); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| /** | ||
| * Copyright (c) OpenLens Authors. All rights reserved. | ||
| * Licensed under MIT License. See LICENSE in root directory for more information. | ||
| */ | ||
|
|
||
| import { getGlobalOverride } from "../test-utils/get-global-override"; | ||
| import ensureDirectoryInjectable from "./ensure-directory.injectable"; | ||
|
|
||
| export default getGlobalOverride(ensureDirectoryInjectable, () => async () => { | ||
| throw new Error("tried to ensure directory without override"); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the requirement for
canvasin the first place? This is complexity which shouldn't be included without very good reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canvas is required for some of the tests IIRC, might not be necessary if we upgrade to xtermJs 5 which adds the DOM based renderer for testing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this complexity, I would say that we stop testing xtermjs directly and only test the contract (our wrapper).