From c80f53a4b543e1e5c4cef7798095fe6e85cf79e2 Mon Sep 17 00:00:00 2001 From: Morgan Martinet Date: Fri, 13 Aug 2021 10:35:08 -0400 Subject: [PATCH] minor refactorings after code review Signed-off-by: Morgan Martinet --- .changeset/tiny-berries-battle.md | 6 +- .../ConfigClusterLocator.test.ts | 4 -- .../cluster-locator/ConfigClusterLocator.ts | 7 +- .../src/cluster-locator/index.test.ts | 2 - .../service/KubernetesFanOutHandler.test.ts | 7 -- .../src/service/KubernetesFanOutHandler.ts | 12 +++- .../KubernetesDrawer/KubernetesDrawer.tsx | 11 +-- .../kubernetes/src/utils/clusterLinks.test.ts | 68 ++++++++++--------- plugins/kubernetes/src/utils/clusterLinks.ts | 36 +++++----- 9 files changed, 79 insertions(+), 74 deletions(-) diff --git a/.changeset/tiny-berries-battle.md b/.changeset/tiny-berries-battle.md index 9e3194a317..fdcb612d61 100644 --- a/.changeset/tiny-berries-battle.md +++ b/.changeset/tiny-berries-battle.md @@ -1,7 +1,7 @@ --- -'@backstage/plugin-kubernetes': minor -'@backstage/plugin-kubernetes-backend': minor -'@backstage/plugin-kubernetes-common': minor +'@backstage/plugin-kubernetes': patch +'@backstage/plugin-kubernetes-backend': patch +'@backstage/plugin-kubernetes-common': patch --- Provide access to the Kubernetes dashboard when viewing a specific resource diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts index cd31db807e..6d1506c4c7 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.test.ts @@ -49,7 +49,6 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { name: 'cluster1', - dashboardUrl: undefined, serviceAccountToken: undefined, url: 'http://localhost:8080', authProvider: 'serviceAccount', @@ -93,7 +92,6 @@ describe('ConfigClusterLocator', () => { }, { name: 'cluster2', - dashboardUrl: undefined, serviceAccountToken: undefined, url: 'http://localhost:8081', authProvider: 'google', @@ -137,7 +135,6 @@ describe('ConfigClusterLocator', () => { expect(result).toStrictEqual([ { assumeRole: undefined, - dashboardUrl: undefined, name: 'cluster1', serviceAccountToken: 'token', externalId: undefined, @@ -147,7 +144,6 @@ describe('ConfigClusterLocator', () => { }, { assumeRole: 'SomeRole', - dashboardUrl: undefined, name: 'cluster2', externalId: undefined, serviceAccountToken: undefined, diff --git a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts index a946871e7b..49ebeaade1 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/ConfigClusterLocator.ts @@ -30,14 +30,17 @@ export class ConfigClusterLocator implements KubernetesClustersSupplier { return new ConfigClusterLocator( config.getConfigArray('clusters').map(c => { const authProvider = c.getString('authProvider'); - const clusterDetails = { + const clusterDetails: ClusterDetails = { name: c.getString('name'), url: c.getString('url'), - dashboardUrl: c.getOptionalString('dashboardUrl'), serviceAccountToken: c.getOptionalString('serviceAccountToken'), skipTLSVerify: c.getOptionalBoolean('skipTLSVerify') ?? false, authProvider: authProvider, }; + const dashboardUrl = c.getOptionalString('dashboardUrl'); + if (dashboardUrl) { + clusterDetails.dashboardUrl = dashboardUrl; + } switch (authProvider) { case 'google': { diff --git a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts index 9b01a4b0f3..95be99a8a5 100644 --- a/plugins/kubernetes-backend/src/cluster-locator/index.test.ts +++ b/plugins/kubernetes-backend/src/cluster-locator/index.test.ts @@ -50,7 +50,6 @@ describe('getCombinedClusterDetails', () => { expect(result).toStrictEqual([ { name: 'cluster1', - dashboardUrl: undefined, serviceAccountToken: 'token', url: 'http://localhost:8080', authProvider: 'serviceAccount', @@ -58,7 +57,6 @@ describe('getCombinedClusterDetails', () => { }, { name: 'cluster2', - dashboardUrl: undefined, serviceAccountToken: undefined, url: 'http://localhost:8081', authProvider: 'google', diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts index 28e8f45603..7973448467 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.test.ts @@ -165,7 +165,6 @@ describe('handleGetKubernetesObjectsForService', () => { items: [ { cluster: { - dashboardUrl: undefined, name: 'test-cluster', }, errors: [], @@ -301,7 +300,6 @@ describe('handleGetKubernetesObjectsForService', () => { }, { cluster: { - dashboardUrl: undefined, name: 'other-cluster', }, errors: [], @@ -400,7 +398,6 @@ describe('handleGetKubernetesObjectsForService', () => { items: [ { cluster: { - dashboardUrl: undefined, name: 'test-cluster', }, errors: [], @@ -439,7 +436,6 @@ describe('handleGetKubernetesObjectsForService', () => { }, { cluster: { - dashboardUrl: undefined, name: 'other-cluster', }, errors: [], @@ -542,7 +538,6 @@ describe('handleGetKubernetesObjectsForService', () => { items: [ { cluster: { - dashboardUrl: undefined, name: 'test-cluster', }, errors: [], @@ -581,7 +576,6 @@ describe('handleGetKubernetesObjectsForService', () => { }, { cluster: { - dashboardUrl: undefined, name: 'other-cluster', }, errors: [], @@ -620,7 +614,6 @@ describe('handleGetKubernetesObjectsForService', () => { }, { cluster: { - dashboardUrl: undefined, name: 'error-cluster', }, errors: ['some random cluster error'], diff --git a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts index 1777d4592a..a1b6f90040 100644 --- a/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts +++ b/plugins/kubernetes-backend/src/service/KubernetesFanOutHandler.ts @@ -22,7 +22,10 @@ import { KubernetesObjectTypes, KubernetesServiceLocator, } from '../types/types'; -import { KubernetesRequestBody } from '@backstage/plugin-kubernetes-common'; +import { + ClusterObjects, + KubernetesRequestBody, +} from '@backstage/plugin-kubernetes-common'; import { KubernetesAuthTranslator } from '../kubernetes-auth-translator/types'; import { KubernetesAuthTranslatorGenerator } from '../kubernetes-auth-translator/KubernetesAuthTranslatorGenerator'; @@ -111,14 +114,17 @@ export class KubernetesFanOutHandler { customResources: this.customResources, }) .then(result => { - return { + const objects: ClusterObjects = { cluster: { name: clusterDetailsItem.name, - dashboardUrl: clusterDetailsItem.dashboardUrl, }, resources: result.responses, errors: result.errors, }; + if (clusterDetailsItem.dashboardUrl) { + objects.cluster.dashboardUrl = clusterDetailsItem.dashboardUrl; + } + return objects; }); }), ).then(r => ({ diff --git a/plugins/kubernetes/src/components/KubernetesDrawer/KubernetesDrawer.tsx b/plugins/kubernetes/src/components/KubernetesDrawer/KubernetesDrawer.tsx index 1fc2f297d3..050d7222d8 100644 --- a/plugins/kubernetes/src/components/KubernetesDrawer/KubernetesDrawer.tsx +++ b/plugins/kubernetes/src/components/KubernetesDrawer/KubernetesDrawer.tsx @@ -34,6 +34,7 @@ import jsYaml from 'js-yaml'; import { CodeSnippet, StructuredMetadataTable, + Link, } from '@backstage/core-components'; import { ClusterContext } from '../../hooks'; import { formatClusterLink } from '../../utils/clusterLinks'; @@ -108,11 +109,11 @@ const KubernetesDrawerContent = ({ const classes = useDrawerContentStyles(); const cluster = useContext(ClusterContext); - const clusterLink = formatClusterLink( - cluster.dashboardUrl ?? '', + const clusterLink = formatClusterLink({ + dashboardUrl: cluster.dashboardUrl, object, kind, - ); + }); return ( <> @@ -150,8 +151,8 @@ const KubernetesDrawerContent = ({ variant="contained" color="primary" size="small" - href={clusterLink} - target="_blank" + component={Link} + to={clusterLink} > Open Kubernetes Dashboard... diff --git a/plugins/kubernetes/src/utils/clusterLinks.test.ts b/plugins/kubernetes/src/utils/clusterLinks.test.ts index d363b35b3c..7c649a34b9 100644 --- a/plugins/kubernetes/src/utils/clusterLinks.test.ts +++ b/plugins/kubernetes/src/utils/clusterLinks.test.ts @@ -19,94 +19,98 @@ import { formatClusterLink } from './clusterLinks'; describe('clusterLinks', () => { describe('formatClusterLink', () => { it('should not return an url when there is no dashboard url', () => { - const url = formatClusterLink('', {}, 'foo'); + const url = formatClusterLink({ object: {}, kind: 'foo' }); expect(url).toBeUndefined(); }); it('should return an url even when there is no object', () => { - const url = formatClusterLink('https://k8s.foo.com', undefined, 'foo'); + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com', + object: undefined, + kind: 'foo', + }); expect(url).toBe('https://k8s.foo.com'); }); it('should return an url on the workloads when there is a namespace only', () => { - const url = formatClusterLink( - 'https://k8s.foo.com', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com', + object: { metadata: { namespace: 'bar', }, }, - 'foo', - ); + kind: 'foo', + }); expect(url).toBe('https://k8s.foo.com/#/workloads?namespace=bar'); }); it('should return an url on the workloads when the kind is not recognizeed', () => { - const url = formatClusterLink( - 'https://k8s.foo.com', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com', + object: { metadata: { name: 'foobar', namespace: 'bar', }, }, - 'UnknownKind', - ); + kind: 'UnknownKind', + }); expect(url).toBe('https://k8s.foo.com/#/workloads?namespace=bar'); }); it('should return an url on the deployment', () => { - const url = formatClusterLink( - 'https://k8s.foo.com/', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com/', + object: { metadata: { name: 'foobar', namespace: 'bar', }, }, - 'Deployment', - ); + kind: 'Deployment', + }); expect(url).toBe( 'https://k8s.foo.com/#/deployment/bar/foobar?namespace=bar', ); }); it('should return an url on the service', () => { - const url = formatClusterLink( - 'https://k8s.foo.com/', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com/', + object: { metadata: { name: 'foobar', namespace: 'bar', }, }, - 'Service', - ); + kind: 'Service', + }); expect(url).toBe( 'https://k8s.foo.com/#/service/bar/foobar?namespace=bar', ); }); it('should return an url on the ingress', () => { - const url = formatClusterLink( - 'https://k8s.foo.com/', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com/', + object: { metadata: { name: 'foobar', namespace: 'bar', }, }, - 'Ingress', - ); + kind: 'Ingress', + }); expect(url).toBe( 'https://k8s.foo.com/#/ingress/bar/foobar?namespace=bar', ); }); it('should return an url on the deployment for a hpa', () => { - const url = formatClusterLink( - 'https://k8s.foo.com/', - { + const url = formatClusterLink({ + dashboardUrl: 'https://k8s.foo.com/', + object: { metadata: { name: 'foobar', namespace: 'bar', }, }, - 'HorizontalPodAutoscaler', - ); + kind: 'HorizontalPodAutoscaler', + }); expect(url).toBe( 'https://k8s.foo.com/#/deployment/bar/foobar?namespace=bar', ); diff --git a/plugins/kubernetes/src/utils/clusterLinks.ts b/plugins/kubernetes/src/utils/clusterLinks.ts index 342c86956f..53f771a1b6 100644 --- a/plugins/kubernetes/src/utils/clusterLinks.ts +++ b/plugins/kubernetes/src/utils/clusterLinks.ts @@ -14,33 +14,37 @@ * limitations under the License. */ -const KindMappings: any = { +const KindMappings: Record = { deployment: 'deployment', ingress: 'ingress', service: 'service', horizontalpodautoscaler: 'deployment', }; -export function formatClusterLink( - dashboardUrl: string, - object: any, - kind: string, -) { - if (!dashboardUrl) { +export function formatClusterLink(options: { + dashboardUrl?: string; + object: any; + kind: string; +}) { + if (!options.dashboardUrl) { return undefined; } - if (!object) { - return dashboardUrl; + if (!options.object) { + return options.dashboardUrl; } - const host = dashboardUrl.endsWith('/') ? dashboardUrl : `${dashboardUrl}/`; - const name = object.metadata?.name; - const namespace = object.metadata?.namespace; - const validKind = KindMappings[kind.toLocaleLowerCase()]; + const host = options.dashboardUrl.endsWith('/') + ? options.dashboardUrl + : `${options.dashboardUrl}/`; + const name = options.object.metadata?.name; + const namespace = options.object.metadata?.namespace; + const validKind = KindMappings[options.kind.toLocaleLowerCase()]; if (validKind && name && namespace) { - return `${host}#/${validKind}/${namespace}/${name}?namespace=${namespace}`; + return `${host}#/${encodeURIComponent(validKind)}/${encodeURIComponent( + namespace, + )}/${encodeURIComponent(name)}?namespace=${encodeURIComponent(namespace)}`; } if (namespace) { - return `${host}#/workloads?namespace=${namespace}`; + return `${host}#/workloads?namespace=${encodeURIComponent(namespace)}`; } - return dashboardUrl; + return options.dashboardUrl; }