From 5d796829bb43ac1771858ef39f757df9bac27768 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fredrik=20Adel=C3=B6w?= Date: Tue, 21 Nov 2023 16:47:03 +0100 Subject: [PATCH] lint for illegal role-to-role dependency chains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fredrik Adelöw --- .changeset/four-needles-watch.md | 5 + .changeset/giant-files-fry.md | 5 + .changeset/warm-plums-beg.md | 5 + plugins/bazaar/package.json | 1 - .../CardContentFields/AboutField.tsx | 75 ++++++++++ .../CardContentFields/CardContentFields.tsx | 2 +- plugins/kubernetes-common/package.json | 1 - .../techdocs-addons-test-utils/package.json | 1 + .../src/test-utils.tsx | 4 +- scripts/verify-local-dependencies.js | 141 +++++++++++++++++- yarn.lock | 3 +- 11 files changed, 229 insertions(+), 14 deletions(-) create mode 100644 .changeset/four-needles-watch.md create mode 100644 .changeset/giant-files-fry.md create mode 100644 .changeset/warm-plums-beg.md create mode 100644 plugins/bazaar/src/components/CardContentFields/AboutField.tsx diff --git a/.changeset/four-needles-watch.md b/.changeset/four-needles-watch.md new file mode 100644 index 0000000000..9c74bce5ad --- /dev/null +++ b/.changeset/four-needles-watch.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-bazaar': patch +--- + +Internalize 'AboutField' to break catalog dependency diff --git a/.changeset/giant-files-fry.md b/.changeset/giant-files-fry.md new file mode 100644 index 0000000000..b01cfbc980 --- /dev/null +++ b/.changeset/giant-files-fry.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-techdocs-addons-test-utils': patch +--- + +Remove unnecessary catalog dependency diff --git a/.changeset/warm-plums-beg.md b/.changeset/warm-plums-beg.md new file mode 100644 index 0000000000..6d09aabd2d --- /dev/null +++ b/.changeset/warm-plums-beg.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-kubernetes-common': patch +--- + +Remove unused dependency diff --git a/plugins/bazaar/package.json b/plugins/bazaar/package.json index c43db667fa..649295ac2b 100644 --- a/plugins/bazaar/package.json +++ b/plugins/bazaar/package.json @@ -34,7 +34,6 @@ "@backstage/core-components": "workspace:^", "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", - "@backstage/plugin-catalog": "workspace:^", "@backstage/plugin-catalog-react": "workspace:^", "@backstage/theme": "workspace:^", "@date-io/luxon": "1.x", diff --git a/plugins/bazaar/src/components/CardContentFields/AboutField.tsx b/plugins/bazaar/src/components/CardContentFields/AboutField.tsx new file mode 100644 index 0000000000..28c22bff37 --- /dev/null +++ b/plugins/bazaar/src/components/CardContentFields/AboutField.tsx @@ -0,0 +1,75 @@ +/* + * Copyright 2020 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useElementFilter } from '@backstage/core-plugin-api'; +import { Grid, makeStyles, Typography } from '@material-ui/core'; +import React from 'react'; + +const useStyles = makeStyles(theme => ({ + value: { + fontWeight: 'bold', + overflow: 'hidden', + lineHeight: '24px', + wordBreak: 'break-word', + }, + label: { + color: theme.palette.text.secondary, + textTransform: 'uppercase', + fontSize: '10px', + fontWeight: 'bold', + letterSpacing: 0.5, + overflow: 'hidden', + whiteSpace: 'nowrap', + }, +})); + +/** + * Props for {@link AboutField}. + * + * @public + */ +export interface AboutFieldProps { + label: string; + value?: string; + gridSizes?: Record; + children?: React.ReactNode; +} + +/** @public */ +export function AboutField(props: AboutFieldProps) { + const { label, value, gridSizes, children } = props; + const classes = useStyles(); + + const childElements = useElementFilter(children, c => c.getElements()); + + // Content is either children or a string prop `value` + const content = + childElements.length > 0 ? ( + childElements + ) : ( + + {value || `unknown`} + + ); + return ( + + + {label} + + {content} + + ); +} diff --git a/plugins/bazaar/src/components/CardContentFields/CardContentFields.tsx b/plugins/bazaar/src/components/CardContentFields/CardContentFields.tsx index 82ab9df89d..f252efebee 100644 --- a/plugins/bazaar/src/components/CardContentFields/CardContentFields.tsx +++ b/plugins/bazaar/src/components/CardContentFields/CardContentFields.tsx @@ -25,8 +25,8 @@ import { import { parseEntityRef } from '@backstage/catalog-model'; import { Avatar, Link } from '@backstage/core-components'; import { useRouteRef } from '@backstage/core-plugin-api'; -import { AboutField } from '@backstage/plugin-catalog'; import { entityRouteRef } from '@backstage/plugin-catalog-react'; +import { AboutField } from './AboutField'; import { StatusTag } from '../StatusTag'; import { Member, BazaarProject } from '../../types'; diff --git a/plugins/kubernetes-common/package.json b/plugins/kubernetes-common/package.json index 1bd5a538d8..d1b04f42f6 100644 --- a/plugins/kubernetes-common/package.json +++ b/plugins/kubernetes-common/package.json @@ -40,7 +40,6 @@ }, "dependencies": { "@backstage/catalog-model": "workspace:^", - "@backstage/core-plugin-api": "workspace:^", "@backstage/errors": "workspace:^", "@backstage/plugin-permission-common": "workspace:^", "@backstage/types": "workspace:^", diff --git a/plugins/techdocs-addons-test-utils/package.json b/plugins/techdocs-addons-test-utils/package.json index b205d9fbcd..856dc718ab 100644 --- a/plugins/techdocs-addons-test-utils/package.json +++ b/plugins/techdocs-addons-test-utils/package.json @@ -38,6 +38,7 @@ "@backstage/core-plugin-api": "workspace:^", "@backstage/integration-react": "workspace:^", "@backstage/plugin-catalog": "workspace:^", + "@backstage/plugin-catalog-react": "workspace:^", "@backstage/plugin-search-react": "workspace:^", "@backstage/plugin-techdocs": "workspace:^", "@backstage/plugin-techdocs-react": "workspace:^", diff --git a/plugins/techdocs-addons-test-utils/src/test-utils.tsx b/plugins/techdocs-addons-test-utils/src/test-utils.tsx index 865e8c0d3b..518e469e1e 100644 --- a/plugins/techdocs-addons-test-utils/src/test-utils.tsx +++ b/plugins/techdocs-addons-test-utils/src/test-utils.tsx @@ -34,7 +34,7 @@ import { techdocsStorageApiRef, } from '@backstage/plugin-techdocs-react'; import { TechDocsReaderPage, techdocsPlugin } from '@backstage/plugin-techdocs'; -import { catalogPlugin } from '@backstage/plugin-catalog'; +import { entityRouteRef } from '@backstage/plugin-catalog-react'; import { searchApiRef } from '@backstage/plugin-search-react'; import { scmIntegrationsApiRef } from '@backstage/integration-react'; @@ -259,7 +259,7 @@ export class TechDocsAddonTester { mountedRoutes: { '/docs': techdocsPlugin.routes.root, '/docs/:namespace/:kind/:name/*': techdocsPlugin.routes.docRoot, - '/catalog/:namespace/:kind/:name': catalogPlugin.routes.catalogEntity, + '/catalog/:namespace/:kind/:name': entityRouteRef, }, }); } diff --git a/scripts/verify-local-dependencies.js b/scripts/verify-local-dependencies.js index b44e0836f4..5d75348681 100755 --- a/scripts/verify-local-dependencies.js +++ b/scripts/verify-local-dependencies.js @@ -33,18 +33,84 @@ const depTypes = [ 'optionalDependencies', ]; +const roleRules = [ + { + sourceRole: ['frontend-plugin', 'web-library'], + targetRole: ['backend-plugin', 'node-library'], + message: `Package SOURCE_NAME with frontend role SOURCE_ROLE has a dependency on package TARGET_NAME with backend role TARGET_ROLE, which is not permitted`, + }, + { + sourceRole: ['backend-plugin', 'node-library'], + targetRole: ['frontend-plugin', 'web-library'], + message: `Package SOURCE_NAME with backend role SOURCE_ROLE has a dependency on package TARGET_NAME with frontend role TARGET_ROLE, which is not permitted`, + }, + { + sourceRole: ['common-library'], + targetRole: [ + 'frontend-plugin', + 'web-library', + 'backend-plugin', + 'node-library', + ], + message: `Polymorphic package SOURCE_NAME has a dependency on package TARGET_NAME with role TARGET_ROLE, which is not permitted since it's not also polymorphic`, + }, + { + sourceRole: ['frontend-plugin', 'web-library'], + targetRole: 'frontend-plugin', + except: [ + // TODO(freben): Address these + '@backstage/plugin-api-docs', + '@backstage/plugin-techdocs-addons-test-utils', + ], + message: `Package SOURCE_NAME with role SOURCE_ROLE has a dependency on another plugin package TARGET_NAME, which is not permitted`, + }, + { + sourceRole: ['frontend-plugin', 'web-library'], + targetName: ['@backstage/core-app-api', '@backstage/frontend-app-api'], + except: [ + // These are legitimate + '@backstage/app-defaults', + '@backstage/core-compat-api', + '@backstage/dev-utils', + '@backstage/frontend-app-api', + '@backstage/frontend-test-utils', + '@backstage/test-utils', + // TODO(freben): Address these + '@backstage/plugin-home', + '@backstage/plugin-techdocs-addons-test-utils', + '@backstage/plugin-user-settings', + ], + message: `Plugin package SOURCE_NAME with role SOURCE_ROLE has a runtime dependency on package TARGET_NAME, which is not permitted. If you are using this dependency for dev server purposes, you can move it to devDependencies instead.`, + }, + { + sourceRole: ['backend-plugin', 'node-library'], + targetName: ['@backstage/backend-app-api'], + except: [ + // These are legitimate + '@backstage/backend-common', + '@backstage/backend-defaults', + '@backstage/backend-test-utils', + ], + message: `Plugin package SOURCE_NAME with role SOURCE_ROLE has a runtime dependency on package TARGET_NAME, which is not permitted. If you are using this dependency for dev server purposes, you can move it to devDependencies instead.`, + }, +]; + async function main(args) { const shouldFix = args.includes('--fix'); const rootPath = resolvePath(__dirname, '..'); const { packages } = await getPackages(rootPath); - let hadErrors = false; + let hadVersionRangeErrors = false; + let hadRoleErrors = false; const pkgMap = new Map(packages.map(pkg => [pkg.packageJson.name, pkg])); for (const pkg of packages) { - let fixed = false; + let versionRangeErrorsFixed = false; + /* + * Ensure that all internal deps have "workspace:^" version ranges + */ for (const depType of depTypes) { const deps = pkg.packageJson[depType]; @@ -54,33 +120,94 @@ async function main(args) { } const localPackage = pkgMap.get(dep); if (localPackage && range !== 'workspace:^') { - hadErrors = true; + hadVersionRangeErrors = true; console.log( `Local dependency from ${pkg.packageJson.name} to ${dep} should have a workspace range`, ); - fixed = true; + versionRangeErrorsFixed = true; pkg.packageJson[depType][dep] = 'workspace:^'; } } } - if (shouldFix && fixed) { + /* + * Ensure that there are no forbidden runtime dependency role combinations + */ + const sourceRole = pkg.packageJson.backstage?.role; + if (typeof sourceRole === 'string') { + for (const [targetName] of Object.entries( + pkg.packageJson.dependencies ?? {}, + )) { + let targetPackageJson; + try { + const packageJsonPath = require.resolve( + `${targetName}/package.json`, + { + paths: [pkg.dir], + }, + ); + targetPackageJson = JSON.parse(await fs.readFile(packageJsonPath)); + } catch { + // ignore + continue; + } + + const sourceName = pkg.packageJson.name; + const targetRole = targetPackageJson.backstage?.role; + if (typeof targetRole === 'string') { + for (const rule of roleRules) { + const matchesSourceRole = [rule.sourceRole ?? []] + .flat() + .includes(sourceRole); + const matchesTargetRole = [rule.targetRole ?? []] + .flat() + .includes(targetRole); + const matchesTargetName = [rule.targetName ?? []] + .flat() + .includes(targetName); + const isExempt = [rule.except ?? []].flat().includes(sourceName); + if ( + matchesSourceRole && + (matchesTargetName || matchesTargetRole) && + !isExempt + ) { + hadRoleErrors = true; + console.error( + rule.message + .replace('SOURCE_NAME', `'${sourceName}'`) + .replace('SOURCE_ROLE', `'${sourceRole}'`) + .replace('TARGET_NAME', `'${targetName}'`) + .replace('TARGET_ROLE', `'${targetRole}'`), + ); + } + } + } + } + } + + /* + * Fixup + */ + if (shouldFix && versionRangeErrorsFixed) { await fs.writeJson(joinPath(pkg.dir, 'package.json'), pkg.packageJson, { spaces: 2, }); } } - if (!shouldFix && hadErrors) { + if (!shouldFix && hadVersionRangeErrors) { console.error(); console.error('At least one package has an invalid local dependency'); console.error( 'Run `node scripts/verify-local-dependencies.js --fix` to fix', ); - process.exit(2); } + + if (hadRoleErrors) { + process.exit(3); + } } main(process.argv.slice(2)).catch(error => { diff --git a/yarn.lock b/yarn.lock index b4cad34800..d3654702fe 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5033,7 +5033,6 @@ __metadata: "@backstage/core-plugin-api": "workspace:^" "@backstage/dev-utils": "workspace:^" "@backstage/errors": "workspace:^" - "@backstage/plugin-catalog": "workspace:^" "@backstage/plugin-catalog-react": "workspace:^" "@backstage/theme": "workspace:^" "@date-io/luxon": 1.x @@ -7365,7 +7364,6 @@ __metadata: "@backstage/catalog-model": "workspace:^" "@backstage/cli": "workspace:^" "@backstage/core-app-api": "workspace:^" - "@backstage/core-plugin-api": "workspace:^" "@backstage/errors": "workspace:^" "@backstage/plugin-permission-common": "workspace:^" "@backstage/test-utils": "workspace:^" @@ -9289,6 +9287,7 @@ __metadata: "@backstage/dev-utils": "workspace:^" "@backstage/integration-react": "workspace:^" "@backstage/plugin-catalog": "workspace:^" + "@backstage/plugin-catalog-react": "workspace:^" "@backstage/plugin-search-react": "workspace:^" "@backstage/plugin-techdocs": "workspace:^" "@backstage/plugin-techdocs-react": "workspace:^"