paulononato/backstage
1
0
Fork 0
Code Issues Pull Requests Actions 25 Packages Projects Releases Wiki Activity

apply code review recommandations

Browse Source 
use encodeURIComponent on dynamic parts
ensure that we keep any dashboard path prefix when generating links
move URL hash exception for SPAs into the standardFormatter

Signed-off-by: Morgan Martinet <morgan@mmm-experts.com>
This commit is contained in:
Morgan Martinet
2021-11-23 18:01:30 -05:00
parent 90ad18e1a7
commit 2c54173a39
8 changed files with 130 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.github/styles/vocab.txt
+1 -1
View File
@@ -189,7 +189,7 @@ oidc
Okta
onboarding
Onboarding
Openshift
OpenShift
orgs
pagerduty
pageview
plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.test.ts
-15
View File
@@ -78,21 +78,6 @@ describe('clusterLinks', () => {
'https://k8s.foo.com/#/service/bar/foobar?namespace=bar',
);
});
it('should return an url on the deployment properly url encoded', () => {
const url = formatClusterLink({
dashboardUrl: 'https://k8s.foo.com/',
object: {
metadata: {
name: 'foobar',
namespace: 'bar bar',
},
},
kind: 'Deployment',
});
expect(url).toBe(
'https://k8s.foo.com/#/deployment/bar%20bar/foobar?namespace=bar+bar',
);
});
});
describe('standard app', () => {
plugins/kubernetes/src/utils/clusterLinks/formatClusterLink.ts
+1 -4
View File
@@ -40,8 +40,5 @@ export function formatClusterLink(options: FormatClusterLinkOptions) {
object: options.object,
kind: options.kind,
});
// Note that we can't rely on 'url.href' since it will put the search before the hash
// and this won't be properly recognized by SPAs such as Angular in the standard dashboard.
// Note also that pathname, hash and search will be properly url encoded.
return `${url.origin}${url.pathname}${url.hash}${url.search}`;
return url.toString();
}
plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.test.ts
+37 -17
View File
@@ -15,11 +15,7 @@
*/
import { openshiftFormatter } from './openshift';
function formatUrl(url: URL) {
return url.toString();
}
describe('clusterLinks - Openshift formatter', () => {
describe('clusterLinks - OpenShift formatter', () => {
it('should return an url on the workloads when there is a namespace only', () => {
const url = openshiftFormatter({
dashboardUrl: new URL('https://k8s.foo.com'),
@@ -30,7 +26,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'foo',
});
expect(formatUrl(url)).toBe('https://k8s.foo.com/k8s/cluster/projects/bar');
expect(url.href).toBe('https://k8s.foo.com/k8s/cluster/projects/bar');
});
it('should return an url on the workloads when the kind is not recognizeed', () => {
const url = openshiftFormatter({
@@ -43,7 +39,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'UnknownKind',
});
expect(formatUrl(url)).toBe('https://k8s.foo.com/k8s/cluster/projects/bar');
expect(url.href).toBe('https://k8s.foo.com/k8s/cluster/projects/bar');
});
it('should return an url on the deployment', () => {
const url = openshiftFormatter({
@@ -56,8 +52,36 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'Deployment',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/k8s/ns/bar/deployments/foobar',
expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/deployments/foobar');
});
it('should return an url on the deployment and keep the path prefix 1', () => {
const url = openshiftFormatter({
dashboardUrl: new URL('https://k8s.foo.com/some/prefix/'),
object: {
metadata: {
name: 'foobar',
namespace: 'bar',
},
},
kind: 'Deployment',
});
expect(url.href).toBe(
'https://k8s.foo.com/some/prefix/k8s/ns/bar/deployments/foobar',
);
});
it('should return an url on the deployment and keep the path prefix 2', () => {
const url = openshiftFormatter({
dashboardUrl: new URL('https://k8s.foo.com/some/prefix'),
object: {
metadata: {
name: 'foobar',
namespace: 'bar',
},
},
kind: 'Deployment',
});
expect(url.href).toBe(
'https://k8s.foo.com/some/prefix/k8s/ns/bar/deployments/foobar',
);
});
it('should return an url on the service', () => {
@@ -71,9 +95,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'Service',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/k8s/ns/bar/services/foobar',
);
expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/services/foobar');
});
it('should return an url on the ingress', () => {
const url = openshiftFormatter({
@@ -86,9 +108,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'Ingress',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/k8s/ns/bar/ingresses/foobar',
);
expect(url.href).toBe('https://k8s.foo.com/k8s/ns/bar/ingresses/foobar');
});
it('should return an url on the deployment for a hpa', () => {
const url = openshiftFormatter({
@@ -101,7 +121,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'HorizontalPodAutoscaler',
});
expect(formatUrl(url)).toBe(
expect(url.href).toBe(
'https://k8s.foo.com/k8s/ns/bar/horizontalpodautoscalers/foobar',
);
});
@@ -115,7 +135,7 @@ describe('clusterLinks - Openshift formatter', () => {
},
kind: 'PersistentVolume',
});
expect(formatUrl(url)).toBe(
expect(url.href).toBe(
'https://k8s.foo.com/k8s/cluster/persistentvolumes/foobar',
);
});
plugins/kubernetes/src/utils/clusterLinks/formatters/openshift.ts
+18 -8
View File
@@ -24,21 +24,31 @@ const kindMappings: Record<string, string> = {
};
export function openshiftFormatter(options: ClusterLinksFormatterOptions): URL {
const result = new URL(options.dashboardUrl.href);
const name = options.object.metadata?.name;
const namespace = options.object.metadata?.namespace;
const basePath = new URL(options.dashboardUrl.href);
const name = encodeURIComponent(options.object.metadata?.name ?? '');
const namespace = encodeURIComponent(
options.object.metadata?.namespace ?? '',
);
const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')];
if (!basePath.pathname.endsWith('/')) {
// a dashboard url with a path should end with a slash otherwise
// the new combined URL will replace the last segment with the appended path!
// https://foobar.com/abc/def + k8s/cluster/projects/test --> https://foobar.com/abc/k8s/cluster/projects/test
// https://foobar.com/abc/def/ + k8s/cluster/projects/test --> https://foobar.com/abc/def/k8s/cluster/projects/test
basePath.pathname += '/';
}
let path = '';
if (namespace) {
if (name && validKind) {
result.pathname = `k8s/ns/${namespace}/${validKind}/${name}`;
path = `k8s/ns/${namespace}/${validKind}/${name}`;
} else {
result.pathname = `k8s/cluster/projects/${namespace}`;
path = `k8s/cluster/projects/${namespace}`;
}
} else if (validKind) {
result.pathname = `k8s/cluster/${validKind}`;
path = `k8s/cluster/${validKind}`;
if (name) {
result.pathname += `/${name}`;
path += `/${name}`;
}
}
return result;
return new URL(path, basePath);
}
plugins/kubernetes/src/utils/clusterLinks/formatters/rancher.ts
+18 -8
View File
@@ -23,14 +23,24 @@ const kindMappings: Record<string, string> = {
};
export function rancherFormatter(options: ClusterLinksFormatterOptions): URL {
const result = new URL(options.dashboardUrl.href);
const name = options.object.metadata?.name;
const namespace = options.object.metadata?.namespace;
const basePath = new URL(options.dashboardUrl.href);
const name = encodeURIComponent(options.object.metadata?.name ?? '');
const namespace = encodeURIComponent(
options.object.metadata?.namespace ?? '',
);
const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')];
if (validKind && name && namespace) {
result.pathname += `explorer/${validKind}/${namespace}/${name}`;
} else if (namespace) {
result.pathname += 'explorer/workload';
if (!basePath.pathname.endsWith('/')) {
// a dashboard url with a path should end with a slash otherwise
// the new combined URL will replace the last segment with the appended path!
// https://foobar.com/abc/def + explorer/service/test --> https://foobar.com/abc/explorer/service/test
// https://foobar.com/abc/def/ + explorer/service/test --> https://foobar.com/abc/def/explorer/service/test
basePath.pathname += '/';
}
return result;
let path = '';
if (validKind && name && namespace) {
path = `explorer/${validKind}/${namespace}/${name}`;
} else if (namespace) {
path = 'explorer/workload';
}
return new URL(path, basePath);
}
plugins/kubernetes/src/utils/clusterLinks/formatters/standard.test.ts
+45
View File
@@ -67,6 +67,51 @@ describe('clusterLinks - standard formatter', () => {
'https://k8s.foo.com/#/deployment/bar/foobar?namespace=bar',
);
});
it('should return an url on the deployment with a prefix 1', () => {
const url = standardFormatter({
dashboardUrl: new URL('https://k8s.foo.com/some/prefix'),
object: {
metadata: {
name: 'foobar',
namespace: 'bar',
},
},
kind: 'Deployment',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/some/prefix/#/deployment/bar/foobar?namespace=bar',
);
});
it('should return an url on the deployment with a prefix 2', () => {
const url = standardFormatter({
dashboardUrl: new URL('https://k8s.foo.com/some/prefix/'),
object: {
metadata: {
name: 'foobar',
namespace: 'bar',
},
},
kind: 'Deployment',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/some/prefix/#/deployment/bar/foobar?namespace=bar',
);
});
it('should return an url on the deployment properly url encoded', () => {
const url = standardFormatter({
dashboardUrl: new URL('https://k8s.foo.com/'),
object: {
metadata: {
name: 'foobar',
namespace: 'bar bar',
},
},
kind: 'Deployment',
});
expect(formatUrl(url)).toBe(
'https://k8s.foo.com/#/deployment/bar%20bar/foobar?namespace=bar%20bar',
);
});
it('should return an url on the service', () => {
const url = standardFormatter({
dashboardUrl: new URL('https://k8s.foo.com/'),
plugins/kubernetes/src/utils/clusterLinks/formatters/standard.ts
+10 -4
View File
@@ -24,16 +24,22 @@ const kindMappings: Record<string, string> = {
export function standardFormatter(options: ClusterLinksFormatterOptions) {
const result = new URL(options.dashboardUrl.href);
const name = options.object.metadata?.name;
const namespace = options.object.metadata?.namespace;
const name = encodeURIComponent(options.object.metadata?.name ?? '');
const namespace = encodeURIComponent(
options.object.metadata?.namespace ?? '',
);
const validKind = kindMappings[options.kind.toLocaleLowerCase('en-US')];
if (namespace) {
result.searchParams.set('namespace', namespace);
if (!result.pathname.endsWith('/')) {
result.pathname += '/';
}
if (validKind && name && namespace) {
result.hash = `/${validKind}/${namespace}/${name}`;
} else if (namespace) {
result.hash = '/workloads';
}
if (namespace) {
// Note that Angular SPA requires a hash and the query parameter should be part of it
result.hash += `?namespace=${namespace}`;
}
return result;
}