Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup Cluster to remove environment specific details #6951

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Cleanup Cluster to remove environment specific details #6951

merged 1 commit into from
Mar 10, 2023

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Jan 16, 2023

Signed-off-by: Sebastian Malton sebastian@malton.name

Please review this commit by commit.

By the end of this PR, the Cluster type will be no more than a shell which contains the status and preferences related details. These can be further re-factored in future PRs.

Notes:

  • All connection related methods that used to be on Cluster are now on a new ClusterConnection type
  • Many things were factored out into keyed singletons to improve readability.

@Nokel81 Nokel81 added the chore label Jan 16, 2023
@Nokel81 Nokel81 added this to the 6.4.0 milestone Jan 16, 2023
@Nokel81 Nokel81 requested a review from a team as a code owner January 16, 2023 21:20
@Nokel81 Nokel81 requested review from ixrock and aleksfront and removed request for a team January 16, 2023 21:20
ixrock
ixrock reviewed Jan 17, 2023
View reviewed changes
src/main/kube-auth-proxy/kube-auth-proxy.ts Outdated Show resolved Hide resolved
src/main/cluster/manager.ts Outdated Show resolved Hide resolved
@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from 1b2d306 to fe1a33d Compare January 17, 2023 20:26
@github-actions
Copy link
Contributor

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch 2 times, most recently from e6b7e70 to 4cdcb66 Compare January 19, 2023 14:42
@github-actions
Copy link
Contributor

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

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Jan 20, 2023

This PR is blocked on #6930 and #4571

@Nokel81 Nokel81 modified the milestones: 6.4.0, 6.5.0 Jan 20, 2023
@github-actions
Copy link
Contributor

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from 9d783fb to 97e5096 Compare March 1, 2023 13:39
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

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

github-advanced-security[bot]
github-advanced-security bot found potential problems Mar 1, 2023
View reviewed changes
packages/core/src/main/cluster-detectors/cluster-distribution-detector.injectable.ts
@@ -13,18 +13,18 @@
const isGKE = (version: string) => version.includes("gke");
const isEKS = (version: string) => version.includes("eks");
const isIKS = (version: string) => version.includes("IKS");
const isAKS = (cluster: Cluster) => cluster.apiUrl.includes("azmk8s.io");
const isAKS = (cluster: Cluster) => cluster.apiUrl.get().includes("azmk8s.io");

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization

'[azmk8s.io](1)' can be anywhere in the URL, and arbitrary hosts may come before or after it.
packages/core/src/main/cluster-detectors/cluster-distribution-detector.injectable.ts
const isMicrok8s = (cluster: Cluster) => cluster.contextName.startsWith("microk8s");
const isKind = (cluster: Cluster) => cluster.contextName.startsWith("kubernetes-admin@kind-");
const isDockerDesktop = (cluster: Cluster) => cluster.contextName === "docker-desktop";
const isDigitalOcean = (cluster: Cluster) => cluster.apiUrl.get().endsWith("k8s.ondigitalocean.com");

Check failure

Code scanning / CodeQL

Incomplete URL substring sanitization

'[k8s.ondigitalocean.com](1)' may be preceded by an arbitrary host name.
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from 665f4c7 to 79b349d Compare March 2, 2023 18:46
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from 65fd3f9 to 37389f8 Compare March 3, 2023 15:40
@github-actions
Copy link
Contributor

github-actions bot commented Mar 3, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from 37389f8 to 9b740b2 Compare March 7, 2023 20:26
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

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

@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from e7987c0 to f14e229 Compare March 9, 2023 20:54
@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

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

@Nokel81
Cleanup 'Cluster' to remove environment specific details
a911f08 
- requestNamespaceListPermissions is infallable so no need to have the extra try/catch

- Refactor isMetricHidden method away from Cluster

- Refactor shouldShowResource out of Cluster

- Refactor isInLocalKubeconfig out of Cluster

- Remove depecrated and unused workspace from Cluster

- Refactor out kubectl as a dependency of Cluster

- Remove from cluster getter used only once

- Split out ClusterConnection from Cluster

- Also split out KubeAuthProxyServer from ContextHandler

- Rename ContextHandler to PrometheusHandler

- Cleanup onNetworkOffline/Online impls within ClusterManager

- Remove annotations from ClusterConnection

- Remove mobx annotations from Cluster

- Rename loadConfigFromFileInjectable

- Remove all uses of dead createClusterInjectionToken

- Fix type errors related to broadcastConnectionUpdate

Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 force-pushed the cleanup-cluster-list-permissions branch from fd42082 to a911f08 Compare March 9, 2023 22:43
jansav
jansav reviewed Mar 10, 2023
View reviewed changes
packages/core/src/common/cluster-store/cluster-store.ts
@@ -64,7 +62,7 @@ export class ClusterStore extends BaseStore<ClusterStoreModel> {

const cluster = clusterOrModel instanceof Cluster
? clusterOrModel
: this.dependencies.createCluster(
: new Cluster(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not good thing to remove the createCluster since it abstracts the fact that Cluster is even a class. new keyword should never be used in the implementation code. There should always be a factory function to abstract the constructor arguments. This allows us to change the implementation how ever we want, but using new Cluster strictly ties us down to Cluster class with specific arguments.

jansav
jansav reviewed Mar 10, 2023
View reviewed changes
packages/core/src/common/cluster/cluster.ts
*/
@observable accessible = false; // if user is able to access cluster resources
readonly accessible = observable.box(false);
Copy link
Contributor

@jansav jansav Mar 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Cluster exposed in the Extension API? This would be breaking change.

This affects all properties that have changed here.

jansav
jansav approved these changes Mar 10, 2023
View reviewed changes
Copy link
Contributor

@jansav jansav left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be said that this PR touches so many places without confidence of unit tests that it is likely to cause some bug somewhere.

Seems like an improvement anyway.

@jansav jansav merged commit ad9bafe into master Mar 10, 2023
@jansav jansav deleted the cleanup-cluster-list-permissions branch March 10, 2023 07:37
@Nokel81 Nokel81 mentioned this pull request Mar 13, 2023
Merged
@aleksfront aleksfront mentioned this pull request Mar 14, 2023
Closed
@Nokel81 Nokel81 mentioned this pull request Mar 14, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ixrock ixrock ixrock left review comments

@jansav jansav jansav approved these changes

@aleksfront aleksfront Awaiting requested review from aleksfront aleksfront is a code owner automatically assigned from lensapp/lens-ide

Assignees
No one assigned
Labels
chore
Projects
None yet
Milestone
6.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Nokel81 @jansav @ixrock