From baf8188db7b1d8aa1df2a208a90e98cc3810ebe1 Mon Sep 17 00:00:00 2001 From: Wojciech Adaszynski Date: Wed, 20 May 2020 09:57:32 +0200 Subject: [PATCH] Sentry plugin refactoring - code review changes --- packages/app/package.json | 2 +- plugins/catalog/package.json | 2 +- plugins/sentry-backend/package.json | 10 +++--- plugins/sentry-backend/src/service/router.ts | 22 +++++------- .../src/service/sentry-api.test.ts | 6 ++-- .../sentry-backend/src/service/sentry-api.ts | 34 +++++++++++-------- plugins/sentry-backend/tsconfig.json | 11 ------ plugins/sentry/package.json | 17 +++++----- .../SentryIssuesTable.test.tsx | 20 +++++------ .../SentryPluginPage.test.tsx | 2 +- .../SentryPluginPage/SentryPluginPage.tsx | 6 ++-- .../SentryPluginWidget/SentryPluginWidget.tsx | 4 +-- plugins/sentry/src/data/production-api.ts | 3 +- plugins/sentry/src/index.ts | 2 +- plugins/sentry/src/setupTests.ts | 2 +- plugins/sentry/tsconfig.json | 5 --- yarn.lock | 10 +++++- 17 files changed, 73 insertions(+), 85 deletions(-) delete mode 100644 plugins/sentry-backend/tsconfig.json delete mode 100644 plugins/sentry/tsconfig.json diff --git a/packages/app/package.json b/packages/app/package.json index 82b25f58e8..0a3d68603b 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -15,7 +15,7 @@ "@backstage/plugin-tech-radar": "^0.1.1-alpha.6", "@backstage/plugin-welcome": "^0.1.1-alpha.6", "@backstage/theme": "^0.1.1-alpha.6", - "@backstage/plugin-sentry": "^0.1.1-alpha.5", + "@backstage/plugin-sentry": "^0.1.1-alpha.6", "@material-ui/core": "^4.9.1", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.45", diff --git a/plugins/catalog/package.json b/plugins/catalog/package.json index fe70b8cc44..cd8548bdde 100644 --- a/plugins/catalog/package.json +++ b/plugins/catalog/package.json @@ -22,7 +22,7 @@ "@material-ui/core": "^4.9.1", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.45", - "@backstage/plugin-sentry": "^0.1.1-alpha.5", + "@backstage/plugin-sentry": "^0.1.1-alpha.6", "react": "^16.13.1", "react-dom": "^16.13.1", "react-use": "^14.2.0" diff --git a/plugins/sentry-backend/package.json b/plugins/sentry-backend/package.json index e9b7d01bf4..34259cdc7b 100644 --- a/plugins/sentry-backend/package.json +++ b/plugins/sentry-backend/package.json @@ -1,6 +1,6 @@ { "name": "@backstage/plugin-sentry-backend", - "version": "0.1.1-alpha.5", + "version": "0.1.1-alpha.6", "main": "dist", "types": "src/index.ts", "license": "Apache-2.0", @@ -13,8 +13,8 @@ "clean": "backstage-cli clean" }, "dependencies": { - "@backstage/backend-common": "^0.1.1-alpha.5", - "@backstage/core": "^0.1.1-alpha.5", + "@backstage/backend-common": "^0.1.1-alpha.6", + "@backstage/core": "^0.1.1-alpha.6", "axios": "^0.19.2", "cors": "^2.8.5", "express": "^4.17.1", @@ -23,8 +23,8 @@ "winston": "^3.2.1" }, "devDependencies": { - "@backstage/cli": "^0.1.1-alpha.5", - "@backstage/dev-utils": "^0.1.1-alpha.5", + "@backstage/cli": "^0.1.1-alpha.6", + "@backstage/dev-utils": "^0.1.1-alpha.6", "@types/jest": "^25.2.1", "@types/node": "^12.0.0" }, diff --git a/plugins/sentry-backend/src/service/router.ts b/plugins/sentry-backend/src/service/router.ts index 3c750bed16..a949f3583f 100644 --- a/plugins/sentry-backend/src/service/router.ts +++ b/plugins/sentry-backend/src/service/router.ts @@ -16,25 +16,19 @@ import { Logger } from 'winston'; import Router from 'express-promise-router'; import express from 'express'; -import { SentryApiForwarder } from './sentry-api'; +import { getSentryApiForwarder } from './sentry-api'; -export async function createRouter( - rootLogger: Logger, -): Promise { +export async function createRouter(logger: Logger): Promise { const router = Router(); const SENTRY_TOKEN = process.env.SENTRY_TOKEN; if (!SENTRY_TOKEN) { - console.error('Sentry token must be provided in env to start the API.'); - process.exit(1); + throw new Error( + 'Sentry token must be provided in SENTRY_TOKEN environment variable to start the API.', + ); } - const sentryForwarder = new SentryApiForwarder(SENTRY_TOKEN); - const logger = rootLogger.child({ plugin: 'sentry' }); + const sentryForwarder = getSentryApiForwarder(SENTRY_TOKEN, logger); - router.get('*', (req, res) => sentryForwarder.fowardRequest(req, res)); + router.use(sentryForwarder); - const app = express(); - app.set('logger', logger); - app.use('/', router); - - return app; + return router; } diff --git a/plugins/sentry-backend/src/service/sentry-api.test.ts b/plugins/sentry-backend/src/service/sentry-api.test.ts index 6473c58a10..f15692861f 100644 --- a/plugins/sentry-backend/src/service/sentry-api.test.ts +++ b/plugins/sentry-backend/src/service/sentry-api.test.ts @@ -13,13 +13,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { SentryApiForwarder } from './sentry-api'; +import { getRequestHeaders } from './sentry-api'; describe('SentryApiForwarder', () => { it('should generate headers based on token passed in constructor', () => { - const forwarder = new SentryApiForwarder('testtoken'); - - expect(forwarder.getRequestHeaders()).toEqual({ + expect(getRequestHeaders('testtoken')).toEqual({ headers: { Authorization: `Bearer testtoken`, }, diff --git a/plugins/sentry-backend/src/service/sentry-api.ts b/plugins/sentry-backend/src/service/sentry-api.ts index 861ad52227..5c8a9f1aa5 100644 --- a/plugins/sentry-backend/src/service/sentry-api.ts +++ b/plugins/sentry-backend/src/service/sentry-api.ts @@ -15,29 +15,33 @@ */ import express from 'express'; import axios from 'axios'; +import { Logger } from 'winston'; -export class SentryApiForwarder { - constructor(private token: string) {} +export function getRequestHeaders(token: string) { + return { + headers: { + Authorization: `Bearer ${token}`, + }, + }; +} - // public for testing - public getRequestHeaders() { - return { - headers: { - Authorization: `Bearer ${this.token}`, - }, - }; - } - public fowardRequest(request: express.Request, response: express.Response) { +export function getSentryApiForwarder(token: string, logger: Logger) { + return function fowardRequest( + request: express.Request, + response: express.Response, + ) { const sentryUrl = request.path; + const effectiveUrl = `https://sentry.io/${sentryUrl}`; + logger.info(`Calling Sentry REST API, ${effectiveUrl}`); axios - .get(`https://sentry.io/${sentryUrl}`, this.getRequestHeaders()) - .then((res) => { + .get(effectiveUrl, getRequestHeaders(token)) + .then(res => { response.send(res.data); }) - .catch((err) => { + .catch(err => { return response.status(err.response.status).json({ detail: err.response.statusText, }); }); - } + }; } diff --git a/plugins/sentry-backend/tsconfig.json b/plugins/sentry-backend/tsconfig.json deleted file mode 100644 index 7d4ea182e2..0000000000 --- a/plugins/sentry-backend/tsconfig.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "extends": "../../packages/backend/tsconfig.json", - "include": [ - "./src" - ], - "compilerOptions": { - "baseUrl": "./src", - "outDir": "./dist", - "skipLibCheck": true - } -} diff --git a/plugins/sentry/package.json b/plugins/sentry/package.json index dde234e901..11decb4087 100644 --- a/plugins/sentry/package.json +++ b/plugins/sentry/package.json @@ -1,7 +1,8 @@ { "name": "@backstage/plugin-sentry", - "version": "0.1.1-alpha.5", + "version": "0.1.1-alpha.6", "main": "dist/index.esm.js", + "main:src": "src/index.ts", "types": "src/index.ts", "license": "Apache-2.0", "private": true, @@ -16,8 +17,8 @@ "clean": "backstage-cli clean" }, "dependencies": { - "@backstage/core": "^0.1.1-alpha.5", - "@backstage/theme": "^0.1.1-alpha.5", + "@backstage/core": "^0.1.1-alpha.6", + "@backstage/theme": "^0.1.1-alpha.6", "@material-ui/core": "^4.9.1", "@material-ui/icons": "^4.9.1", "@material-ui/lab": "4.0.0-alpha.45", @@ -28,12 +29,12 @@ "timeago.js": "^4.0.2" }, "devDependencies": { - "@backstage/cli": "^0.1.1-alpha.5", - "@backstage/dev-utils": "^0.1.1-alpha.5", - "@testing-library/jest-dom": "^4.2.4", + "@backstage/cli": "^0.1.1-alpha.6", + "@backstage/dev-utils": "^0.1.1-alpha.6", + "@testing-library/jest-dom": "^5.7.0", "@testing-library/react": "^9.3.2", - "@testing-library/user-event": "^7.1.2", - "@types/jest": "^25.2.1", + "@testing-library/user-event": "^10.2.4", + "@types/jest": "^25.2.2", "@types/node": "^12.0.0", "@types/testing-library__jest-dom": "^5.0.4", "jest-fetch-mock": "^3.0.3" diff --git a/plugins/sentry/src/components/SentryIssuesTable/SentryIssuesTable.test.tsx b/plugins/sentry/src/components/SentryIssuesTable/SentryIssuesTable.test.tsx index 4bd231a1e5..6926fdfc2a 100644 --- a/plugins/sentry/src/components/SentryIssuesTable/SentryIssuesTable.test.tsx +++ b/plugins/sentry/src/components/SentryIssuesTable/SentryIssuesTable.test.tsx @@ -39,12 +39,12 @@ describe('SentryIssuesTable', () => { , ); - expect(await table.findByText('Error')).toBeInTheDOM(); - expect(await table.findByText('Graph')).toBeInTheDOM(); - expect(await table.findByText('First seen')).toBeInTheDOM(); - expect(await table.findByText('Last seen')).toBeInTheDOM(); - expect(await table.findByText('Events')).toBeInTheDOM(); - expect(await table.findByText('Users')).toBeInTheDOM(); + expect(await table.findByText('Error')).toBeInTheDocument(); + expect(await table.findByText('Graph')).toBeInTheDocument(); + expect(await table.findByText('First seen')).toBeInTheDocument(); + expect(await table.findByText('Last seen')).toBeInTheDocument(); + expect(await table.findByText('Events')).toBeInTheDocument(); + expect(await table.findByText('Users')).toBeInTheDocument(); }); it('should render values in a table', async () => { const issues: SentryIssue[] = [ @@ -63,9 +63,9 @@ describe('SentryIssuesTable', () => { , ); - expect(await table.findByText('Exception')).toBeInTheDOM(); - expect(await table.findByText('exception was thrown')).toBeInTheDOM(); - expect(await table.findByText('101')).toBeInTheDOM(); - expect(await table.findByText('202')).toBeInTheDOM(); + expect(await table.findByText('Exception')).toBeInTheDocument(); + expect(await table.findByText('exception was thrown')).toBeInTheDocument(); + expect(await table.findByText('101')).toBeInTheDocument(); + expect(await table.findByText('202')).toBeInTheDocument(); }); }); diff --git a/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.test.tsx b/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.test.tsx index 400c75e065..b0437ff4f7 100644 --- a/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.test.tsx +++ b/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.test.tsx @@ -26,7 +26,7 @@ const errorApi = { post: () => {} }; describe('SentryPluginPage', () => { it('should render header and time switched', () => { - mockFetch.mockResponse(() => new Promise(() => {})); + mockFetch.mockResponse('{}'); const rendered = render( diff --git a/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.tsx b/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.tsx index d7bf731a93..d8dc55a800 100644 --- a/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.tsx +++ b/plugins/sentry/src/components/SentryPluginPage/SentryPluginPage.tsx @@ -24,13 +24,13 @@ import { ContentHeader, SupportButton, } from '@backstage/core'; -import SentryPluginWidget from '../SentryPluginWidget/SentryPluginWidget'; +import { SentryPluginWidget } from '../SentryPluginWidget/SentryPluginWidget'; import { ToggleButton, ToggleButtonGroup } from '@material-ui/lab'; const SentryPluginPage: FC<{}> = () => { const [statsFor, setStatsFor] = useState<'12h' | '24h'>('12h'); - const toggleStatsFor = () => - statsFor === '12h' ? setStatsFor('24h') : setStatsFor('12h'); + const toggleStatsFor = () => setStatsFor(statsFor === '12h' ? '12h' : '24h'); + return (
diff --git a/plugins/sentry/src/components/SentryPluginWidget/SentryPluginWidget.tsx b/plugins/sentry/src/components/SentryPluginWidget/SentryPluginWidget.tsx index 9b5131d01e..5049905966 100644 --- a/plugins/sentry/src/components/SentryPluginWidget/SentryPluginWidget.tsx +++ b/plugins/sentry/src/components/SentryPluginWidget/SentryPluginWidget.tsx @@ -28,7 +28,7 @@ import { sentryApiFactory } from '../../data/api-factory'; const api = sentryApiFactory('spotify'); -const SentryPluginWidget: FC<{ +export const SentryPluginWidget: FC<{ sentryProjectId: string; statsFor: '24h' | '12h'; }> = ({ sentryProjectId, statsFor }) => { @@ -55,5 +55,3 @@ const SentryPluginWidget: FC<{ return ; }; - -export default SentryPluginWidget; diff --git a/plugins/sentry/src/data/production-api.ts b/plugins/sentry/src/data/production-api.ts index 0e8f22452f..ce4bab5aca 100644 --- a/plugins/sentry/src/data/production-api.ts +++ b/plugins/sentry/src/data/production-api.ts @@ -16,7 +16,8 @@ import { SentryIssue } from './sentry-issue'; import { SentryApi } from './sentry-api'; -const API_BASE_URL = 'http://localhost:7000/sentry/api/0/projects/'; +const API_HOST = process.env.API_HOST || 'http://localhost:7000'; +const API_BASE_URL = `${API_HOST}/sentry/api/0/projects/`; export class ProductionSentryApi implements SentryApi { private organization: string; diff --git a/plugins/sentry/src/index.ts b/plugins/sentry/src/index.ts index 350b8fd668..50da801a40 100644 --- a/plugins/sentry/src/index.ts +++ b/plugins/sentry/src/index.ts @@ -15,4 +15,4 @@ */ export { plugin } from './plugin'; -export { default as SentryIssuesWidget } from './components/SentryPluginWidget/SentryPluginWidget'; +export { SentryPluginWidget as SentryIssuesWidget } from './components/SentryPluginWidget/SentryPluginWidget'; diff --git a/plugins/sentry/src/setupTests.ts b/plugins/sentry/src/setupTests.ts index 1a907ab8e6..e34bc46f4b 100644 --- a/plugins/sentry/src/setupTests.ts +++ b/plugins/sentry/src/setupTests.ts @@ -14,5 +14,5 @@ * limitations under the License. */ -import '@testing-library/jest-dom/extend-expect'; +import '@testing-library/jest-dom'; require('jest-fetch-mock').enableMocks(); diff --git a/plugins/sentry/tsconfig.json b/plugins/sentry/tsconfig.json deleted file mode 100644 index b663b01fa2..0000000000 --- a/plugins/sentry/tsconfig.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "extends": "../../tsconfig.json", - "include": ["src", "dev"], - "compilerOptions": {} -} diff --git a/yarn.lock b/yarn.lock index 0e0edd4bf3..245e4fb7c9 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4015,6 +4015,14 @@ jest-diff "^25.2.1" pretty-format "^25.2.1" +"@types/jest@^25.2.1": + version "25.2.3" + resolved "https://registry.npmjs.org/@types/jest/-/jest-25.2.3.tgz#33d27e4c4716caae4eced355097a47ad363fdcaf" + integrity sha512-JXc1nK/tXHiDhV55dvfzqtmP4S3sy3T3ouV2tkViZgxY/zeUkcpQcQPGRlgF4KmWzWW5oiWYSZwtCB+2RsE4Fw== + dependencies: + jest-diff "^25.2.1" + pretty-format "^25.2.1" + "@types/jest@^25.2.2": version "25.2.2" resolved "https://registry.npmjs.org/@types/jest/-/jest-25.2.2.tgz#6a752e7a00f69c3e790ea00c345029d5cefa92bf" @@ -5322,7 +5330,7 @@ aws4@^1.8.0: resolved "https://registry.npmjs.org/aws4/-/aws4-1.9.1.tgz#7e33d8f7d449b3f673cd72deb9abdc552dbe528e" integrity sha512-wMHVg2EOHaMRxbzgFJ9gtjOOCrI80OHLG14rxi28XwOW8ux6IiEbRCGGGqCtdAIg4FQCbW20k9RsT4y3gJlFug== -axios@^0.19.0: +axios@^0.19.0, axios@^0.19.2: version "0.19.2" resolved "https://registry.npmjs.org/axios/-/axios-0.19.2.tgz#3ea36c5d8818d0d5f8a8a97a6d36b86cdc00cb27" integrity sha512-fjgm5MvRHLhx+osE2xoekY70AhARk3a6hkN+3Io1jc00jtquGvxYlKlsFUhmUET0V5te6CcZI7lcv2Ym61mjHA==