From cb09872169922a13dcccb889f0857927585d2405 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Sat, 6 Jan 2024 14:50:46 +1100 Subject: [PATCH 01/13] Uplift internal linting --- .changeset/five-dancers-pump.md | 12 ++ src/cli/__snapshots__/format.int.test.ts.snap | 24 +++ src/cli/__snapshots__/lint.int.test.ts.snap | 44 +++--- src/cli/configure/refreshIgnoreFiles.ts | 57 ------- src/cli/format.ts | 10 +- src/cli/lint.int.test.ts | 28 +++- src/cli/lint/autofix.test.ts | 122 +++++---------- src/cli/lint/autofix.ts | 102 +++---------- src/cli/lint/external.ts | 10 +- src/cli/lint/index.ts | 15 +- src/cli/lint/internal.test.ts | 15 +- src/cli/lint/internal.ts | 103 ++++++++++--- .../lint/internalLints/deleteFiles.test.ts | 99 ++++++++++++ src/cli/lint/internalLints/deleteFiles.ts | 81 ++++++++++ .../internalLints/noSkubaTemplateJs.test.ts | 61 ++++++++ .../lint/internalLints/noSkubaTemplateJs.ts | 41 +++++ .../internalLints/refreshIgnoreFiles.test.ts | 143 ++++++++++++++++++ .../lint/internalLints/refreshIgnoreFiles.ts | 113 ++++++++++++++ .../{configure => lint}/upgrade/index.test.ts | 110 +++++++++++++- src/cli/{configure => lint}/upgrade/index.ts | 54 +++++-- .../patches/7.3.1/addEmptyExports.test.ts | 4 +- .../upgrade/patches/7.3.1/addEmptyExports.ts | 9 +- .../upgrade/patches/7.3.1/index.ts | 2 +- .../patches/7.3.1/patchDockerfile.test.ts | 0 .../upgrade/patches/7.3.1/patchDockerfile.ts | 2 +- .../patches/7.3.1/patchServerListener.test.ts | 0 .../patches/7.3.1/patchServerListener.ts | 9 +- src/utils/logging.ts | 1 + 28 files changed, 953 insertions(+), 318 deletions(-) create mode 100644 .changeset/five-dancers-pump.md delete mode 100644 src/cli/configure/refreshIgnoreFiles.ts create mode 100644 src/cli/lint/internalLints/deleteFiles.test.ts create mode 100644 src/cli/lint/internalLints/deleteFiles.ts create mode 100644 src/cli/lint/internalLints/noSkubaTemplateJs.test.ts create mode 100644 src/cli/lint/internalLints/noSkubaTemplateJs.ts create mode 100644 src/cli/lint/internalLints/refreshIgnoreFiles.test.ts create mode 100644 src/cli/lint/internalLints/refreshIgnoreFiles.ts rename src/cli/{configure => lint}/upgrade/index.test.ts (57%) rename src/cli/{configure => lint}/upgrade/index.ts (60%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/addEmptyExports.test.ts (94%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/addEmptyExports.ts (82%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/index.ts (80%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/patchDockerfile.test.ts (100%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/patchDockerfile.ts (91%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/patchServerListener.test.ts (100%) rename src/cli/{configure => lint}/upgrade/patches/7.3.1/patchServerListener.ts (85%) diff --git a/.changeset/five-dancers-pump.md b/.changeset/five-dancers-pump.md new file mode 100644 index 000000000..c04f583fb --- /dev/null +++ b/.changeset/five-dancers-pump.md @@ -0,0 +1,12 @@ +--- +'skuba': minor +--- + +lint: Overhaul skuba's internal linting system + +Internal (skuba) linting is now promoted to a top-level tool alongside ESLint, tsc, and Prettier. + +This fixes issues where skuba would not fail a `lint` check but silently make changes. +These changes may never end up being committed and causes noise when running `lint` or `format` later. + +Now, lints report whether changes need to be made and are applied in `format` or autofix modes (in CI). diff --git a/src/cli/__snapshots__/format.int.test.ts.snap b/src/cli/__snapshots__/format.int.test.ts.snap index c53e4adb2..4fd545bb1 100644 --- a/src/cli/__snapshots__/format.int.test.ts.snap +++ b/src/cli/__snapshots__/format.int.test.ts.snap @@ -2,12 +2,18 @@ exports[`fixable 1`] = ` " +skuba lints +Refreshed .eslintignore. refresh-ignore-files +Refreshed .gitignore. refresh-ignore-files +Refreshed .prettierignore. refresh-ignore-files Updating skuba... Patch 7.3.1 applied. skuba update complete. +Processed skuba lints in s. + ESLint Processed 4 files in s. @@ -57,12 +63,18 @@ d.js exports[`ok --debug 1`] = ` " +skuba lints +Refreshed .eslintignore. refresh-ignore-files +Refreshed .gitignore. refresh-ignore-files +Refreshed .prettierignore. refresh-ignore-files Updating skuba... Patch 7.3.1 applied. skuba update complete. +Processed skuba lints in s. + ESLint Initialising ESLint... Processing files... @@ -105,12 +117,18 @@ exports[`ok --debug 2`] = ` exports[`ok 1`] = ` " +skuba lints +Refreshed .eslintignore. refresh-ignore-files +Refreshed .gitignore. refresh-ignore-files +Refreshed .prettierignore. refresh-ignore-files Updating skuba... Patch 7.3.1 applied. skuba update complete. +Processed skuba lints in s. + ESLint Processed 2 files in s. @@ -126,12 +144,18 @@ exports[`ok 2`] = ` exports[`unfixable 1`] = ` " +skuba lints +Refreshed .eslintignore. refresh-ignore-files +Refreshed .gitignore. refresh-ignore-files +Refreshed .prettierignore. refresh-ignore-files Updating skuba... Patch 7.3.1 applied. skuba update complete. +Processed skuba lints in s. + ESLint Processed 2 files in s. diff --git a/src/cli/__snapshots__/lint.int.test.ts.snap b/src/cli/__snapshots__/lint.int.test.ts.snap index 16a42464f..e078ccc9d 100644 --- a/src/cli/__snapshots__/lint.int.test.ts.snap +++ b/src/cli/__snapshots__/lint.int.test.ts.snap @@ -2,12 +2,6 @@ exports[`fixable 1`] = ` " -Updating skuba... - -Patch 7.3.1 applied. - -skuba update complete. - ESLint │ Processed 4 files in s. ESLint │ /a/a/a.mjs @@ -23,14 +17,16 @@ ESLint │ 3 errors and 0 warnings potentially fixable with the \`--fix\` option. Prettier │ Processed 8 files in s. -Prettier │ Flagged 3 files: +Prettier │ Flagged 4 files: Prettier │ b.md Prettier │ c.json Prettier │ d.js +Prettier │ package.json tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 0 ESLint, Prettier found issues that require triage. +skuba │ Processed skuba lints in s. " `; @@ -67,19 +63,28 @@ Options: { b.md c.json d.js +package.json \`\`\` ", ] `; -exports[`ok --debug 1`] = ` +exports[`needs patches 1`] = ` " -Updating skuba... - -Patch 7.3.1 applied. +ESLint │ Processed 2 files in s. +Prettier │ Processed 6 files in s. +tsc │ TSFILE: /lib/tsconfig.tsbuildinfo +tsc │ tsc --noEmit exited with code 0 +skuba │ skuba has patches to apply. Run yarn skuba format to run them. skuba-patches +skuba │ Some skuba lints failed. Try running yarn skuba format to fix them. skuba-lint +skuba │ Processed skuba lints in s. +" +`; -skuba update complete. +exports[`needs patches 2`] = `[]`; +exports[`ok --debug 1`] = ` +" ESLint │ Initialising ESLint... ESLint │ Processing files... ESLint │ Processed 2 files in s. @@ -139,6 +144,7 @@ tsc │ printTime time: s tsc │ Emit time: s tsc │ Total time: s tsc │ tsc --extendedDiagnostics --noEmit exited with code 0 +skuba │ Processed skuba lints in s. " `; @@ -146,16 +152,11 @@ exports[`ok --debug 2`] = `[]`; exports[`ok 1`] = ` " -Updating skuba... - -Patch 7.3.1 applied. - -skuba update complete. - ESLint │ Processed 2 files in s. Prettier │ Processed 6 files in s. tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 0 +skuba │ Processed skuba lints in s. " `; @@ -163,12 +164,6 @@ exports[`ok 2`] = `[]`; exports[`unfixable 1`] = ` " -Updating skuba... - -Patch 7.3.1 applied. - -skuba update complete. - ESLint │ Processed 2 files in s. ESLint │ /a/a/a.ts @@ -193,6 +188,7 @@ tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 2 ESLint, Prettier, tsc found issues that require triage. +skuba │ Processed skuba lints in s. " `; diff --git a/src/cli/configure/refreshIgnoreFiles.ts b/src/cli/configure/refreshIgnoreFiles.ts deleted file mode 100644 index a18c469b8..000000000 --- a/src/cli/configure/refreshIgnoreFiles.ts +++ /dev/null @@ -1,57 +0,0 @@ -import path from 'path'; -import { inspect } from 'util'; - -import fs from 'fs-extra'; - -import { log } from '../../utils/logging'; -import { readBaseTemplateFile } from '../../utils/template'; - -import { getDestinationManifest } from './analysis/package'; -import { createDestinationFileReader } from './analysis/project'; -import { mergeWithIgnoreFile } from './processing/ignoreFile'; - -export const REFRESHABLE_IGNORE_FILES = [ - '.eslintignore', - '.gitignore', - '.prettierignore', -]; - -export const refreshIgnoreFiles = async () => { - // TODO: check current state of .gitignore - // If it contains !.npmrc, break - // If it contains .npmrc, we can either - // 1. Move the entry below the skuba-managed section for manual triage - // 2. Delete any local .npmrc state before un-ignoring the .npmrc - - const manifest = await getDestinationManifest(); - - const destinationRoot = path.dirname(manifest.path); - - const readDestinationFile = createDestinationFileReader(destinationRoot); - - const refreshIgnoreFile = async (filename: string) => { - const [inputFile, templateFile] = await Promise.all([ - readDestinationFile(filename), - readBaseTemplateFile(`_${filename}`), - ]); - - const data = inputFile - ? mergeWithIgnoreFile(templateFile)(inputFile) - : templateFile; - - const filepath = path.join(destinationRoot, filename); - - await fs.promises.writeFile(filepath, data); - }; - - await Promise.all(REFRESHABLE_IGNORE_FILES.map(refreshIgnoreFile)); -}; - -export const tryRefreshIgnoreFiles = async () => { - try { - await refreshIgnoreFiles(); - } catch (err) { - log.warn('Failed to refresh ignore files.'); - log.subtle(inspect(err)); - } -}; diff --git a/src/cli/format.ts b/src/cli/format.ts index 8a91d9c69..8ababfd76 100644 --- a/src/cli/format.ts +++ b/src/cli/format.ts @@ -5,15 +5,16 @@ import { createLogger, log } from '../utils/logging'; import { runESLint } from './adapter/eslint'; import { runPrettier } from './adapter/prettier'; -import { tryRefreshIgnoreFiles } from './configure/refreshIgnoreFiles'; -import { upgradeSkuba } from './configure/upgrade'; +import { internalLint } from './lint/internal'; export const format = async (args = process.argv.slice(2)): Promise => { - await Promise.all([tryRefreshIgnoreFiles(), upgradeSkuba()]); + log.plain(chalk.blueBright('skuba lints')); + const internal = await internalLint('format', { debug: false, serial: true }); const debug = hasDebugFlag(args); const logger = createLogger(debug); + log.newline(); log.plain(chalk.magenta('ESLint')); const eslint = await runESLint('format', logger); @@ -23,13 +24,14 @@ export const format = async (args = process.argv.slice(2)): Promise => { const prettier = await runPrettier('format', logger); - if (eslint.ok && prettier.ok) { + if (eslint.ok && prettier.ok && internal.ok) { return; } const tools = [ ...(eslint.ok ? [] : ['ESLint']), ...(prettier.ok ? [] : ['Prettier']), + ...(internal.ok ? [] : ['skuba']), ]; log.newline(); diff --git a/src/cli/lint.int.test.ts b/src/cli/lint.int.test.ts index 2241267e9..8165e5d08 100644 --- a/src/cli/lint.int.test.ts +++ b/src/cli/lint.int.test.ts @@ -7,11 +7,16 @@ import fs, { copy } from 'fs-extra'; import git from 'isomorphic-git'; import { Buildkite } from '..'; +import type { Logger } from '../utils/logging'; +import { getSkubaVersion } from '../utils/version'; import { lint } from './lint'; +import { refreshIgnoreFiles } from './lint/internalLints/refreshIgnoreFiles'; jest.setTimeout(30_000); +jest.mock('../utils/version'); + const buildkiteAnnotate = jest.spyOn(Buildkite, 'annotate').mockResolvedValue(); const stdoutMock = jest.fn(); @@ -64,8 +69,13 @@ const stdout = (randomMatcher: RegExp) => { const prepareTempDirectory = async (baseDir: string, tempDir: string) => { await copy(baseDir, tempDir); - process.chdir(tempDir); + const result = await refreshIgnoreFiles('format', { + bold: jest.fn(), + dim: jest.fn(), + warn: jest.fn(), + } as unknown as Logger); + expect(result.ok).toBe(true); }; const originalCwd = process.cwd(); @@ -100,16 +110,20 @@ afterAll(() => { interface Args { args: string[]; base: string; + skubaVersion: string; exitCode: number | undefined; } test.each` - description | args | base | exitCode - ${'fixable'} | ${[]} | ${'fixable'} | ${1} - ${'ok'} | ${[]} | ${'ok'} | ${undefined} - ${'ok --debug'} | ${['--debug']} | ${'ok'} | ${undefined} - ${'unfixable'} | ${[]} | ${'unfixable'} | ${1} -`('$description', async ({ args, base, exitCode }: Args) => { + description | args | base | skubaVersion | exitCode + ${'fixable'} | ${[]} | ${'fixable'} | ${'0.0.0'} | ${1} + ${'ok'} | ${[]} | ${'ok'} | ${'0.0.0'} | ${undefined} + ${'ok --debug'} | ${['--debug']} | ${'ok'} | ${'0.0.0'} | ${undefined} + ${'unfixable'} | ${[]} | ${'unfixable'} | ${'0.0.0'} | ${1} + ${'needs patches'} | ${[]} | ${'ok'} | ${'1.0.0'} | ${1} +`('$description', async ({ args, base, skubaVersion, exitCode }: Args) => { + jest.mocked(getSkubaVersion).mockResolvedValue(skubaVersion); + const baseDir = path.join(BASE_PATH, base); const tempDir = path.join( diff --git a/src/cli/lint/autofix.test.ts b/src/cli/lint/autofix.test.ts index 55ef35fa4..c70267977 100644 --- a/src/cli/lint/autofix.test.ts +++ b/src/cli/lint/autofix.test.ts @@ -7,6 +7,7 @@ import { runESLint } from '../adapter/eslint'; import { runPrettier } from '../adapter/prettier'; import { AUTOFIX_IGNORE_FILES, autofix } from './autofix'; +import { internalLint } from './internal'; jest.mock('simple-git'); jest.mock('../../api/git'); @@ -14,6 +15,7 @@ jest.mock('../../api/github'); jest.mock('../../api/buildkite'); jest.mock('../adapter/eslint'); jest.mock('../adapter/prettier'); +jest.mock('./internal'); const MOCK_ERROR = new Error('Badness!'); @@ -44,19 +46,25 @@ beforeEach(() => { afterEach(jest.resetAllMocks); describe('autofix', () => { - const params = { debug: false, eslint: true, prettier: true }; + const params = { + debug: false, + eslint: true, + prettier: true, + internal: true, + }; describe('GitHub Actions', () => { const push = jest.fn(); const expectAutofixCommit = ( - { eslint, prettier }: Record<'eslint' | 'prettier', boolean> = { + { eslint, internal }: Record<'eslint' | 'internal', boolean> = { eslint: true, - prettier: true, + internal: true, }, ) => { expect(runESLint).toHaveBeenCalledTimes(eslint ? 1 : 0); - expect(runPrettier).toHaveBeenCalledTimes(prettier ? 1 : 0); + expect(runPrettier).toHaveBeenCalledTimes(1); + expect(internalLint).toHaveBeenCalledTimes(internal ? 1 : 0); expect(Git.commitAllChanges).toHaveBeenCalledTimes(1); }; @@ -156,7 +164,7 @@ describe('autofix', () => { it('bails on no fixable issues', async () => { await expect( - autofix({ ...params, eslint: false, prettier: false }), + autofix({ ...params, eslint: false, prettier: false, internal: false }), ).resolves.toBeUndefined(); expectNoAutofix(); @@ -173,7 +181,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... No autofixes detected. " `); @@ -200,7 +208,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -222,7 +230,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -233,10 +241,10 @@ describe('autofix', () => { jest.mocked(Git.currentBranch).mockResolvedValue('dev'); await expect( - autofix({ ...params, eslint: false, prettier: true }), + autofix({ ...params, eslint: false, internal: false, prettier: true }), ).resolves.toBeUndefined(); - expectAutofixCommit({ eslint: false, prettier: true }); + expectAutofixCommit({ eslint: false, internal: false }); expect(Git.commitAllChanges).toHaveBeenNthCalledWith(1, { dir: expect.any(String), @@ -251,13 +259,13 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with Prettier... + Attempting to autofix issues (Prettier)... Pushed fix commit commit-sha. " `); }); - it('handles codegen changes only', async () => { + it('handles internal changes only', async () => { jest.spyOn(Git, 'getChangedFiles').mockResolvedValue([ { path: '.gitignore', @@ -269,10 +277,10 @@ describe('autofix', () => { jest.mocked(Git.currentBranch).mockResolvedValue('dev'); await expect( - autofix({ ...params, eslint: false, prettier: false }), + autofix({ ...params, eslint: false, prettier: false, internal: true }), ).resolves.toBeUndefined(); - expectAutofixCommit({ eslint: false, prettier: false }); + expectAutofixCommit({ eslint: false, internal: true }); expect(Git.commitAllChanges).toHaveBeenNthCalledWith(1, { dir: expect.any(String), @@ -286,42 +294,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to push codegen updates... - Pushed fix commit commit-sha. - " - `); - }); - - it('handles nested Renovate changes only', async () => { - jest.spyOn(Git, 'getChangedFiles').mockResolvedValue([ - { - path: '.github/renovate.json5', - state: 'modified', - }, - ]); - - jest.mocked(Git.commitAllChanges).mockResolvedValue('commit-sha'); - jest.mocked(Git.currentBranch).mockResolvedValue('dev'); - - await expect( - autofix({ ...params, eslint: false, prettier: false }), - ).resolves.toBeUndefined(); - - expectAutofixCommit({ eslint: false, prettier: false }); - - expect(Git.commitAllChanges).toHaveBeenNthCalledWith(1, { - dir: expect.any(String), - message: 'Run `skuba format`', - - ignore: AUTOFIX_IGNORE_FILES, - }); - - expect(push).toHaveBeenNthCalledWith(1); - - expect(stdout()).toMatchInlineSnapshot(` - " - - Trying to push codegen updates... + Attempting to autofix issues (skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -343,7 +316,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -360,7 +333,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Failed to push fix commit. Does your CI environment have write access to your Git repository? Error: Badness! @@ -371,19 +344,21 @@ describe('autofix', () => { describe('Other CI', () => { const expectAutofixCommit = ( - { eslint, prettier }: Record<'eslint' | 'prettier', boolean> = { + { eslint, internal }: Record<'eslint' | 'internal', boolean> = { eslint: true, - prettier: true, + internal: true, }, ) => { expect(runESLint).toHaveBeenCalledTimes(eslint ? 1 : 0); - expect(runPrettier).toHaveBeenCalledTimes(prettier ? 1 : 0); + expect(runPrettier).toHaveBeenCalledTimes(1); + expect(internalLint).toHaveBeenCalledTimes(internal ? 1 : 0); expect(GitHub.uploadAllFileChanges).toHaveBeenCalledTimes(1); }; const expectNoAutofix = () => { expect(runESLint).not.toHaveBeenCalled(); expect(runPrettier).not.toHaveBeenCalled(); + expect(internalLint).not.toHaveBeenCalled(); expect(GitHub.uploadAllFileChanges).not.toHaveBeenCalled(); }; @@ -446,7 +421,7 @@ describe('autofix', () => { jest.mocked(Git.currentBranch).mockResolvedValue('feature'); await expect( - autofix({ ...params, eslint: false, prettier: false }), + autofix({ ...params, eslint: false, prettier: false, internal: false }), ).resolves.toBeUndefined(); expectNoAutofix(); @@ -462,7 +437,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... No autofixes detected. " `); @@ -490,7 +465,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -501,10 +476,10 @@ describe('autofix', () => { jest.mocked(Git.currentBranch).mockResolvedValue('dev'); await expect( - autofix({ ...params, eslint: false, prettier: true }), + autofix({ ...params, eslint: false, internal: false, prettier: true }), ).resolves.toBeUndefined(); - expectAutofixCommit({ eslint: false, prettier: true }); + expectAutofixCommit({ eslint: false, internal: false }); expect(GitHub.uploadAllFileChanges).toHaveBeenNthCalledWith(1, { branch: 'dev', @@ -518,28 +493,13 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with Prettier... + Attempting to autofix issues (Prettier)... Pushed fix commit commit-sha. " `); }); - it('skips an .npmrc modification only', async () => { - jest.spyOn(Git, 'getChangedFiles').mockResolvedValue([ - { - path: '.npmrc', - state: 'modified', - }, - ]); - - await expect( - autofix({ ...params, eslint: false, prettier: false }), - ).resolves.toBeUndefined(); - - expectNoAutofix(); - }); - - it('handles codegen changes only', async () => { + it('handles internal changes only', async () => { jest.spyOn(Git, 'getChangedFiles').mockResolvedValue([ { path: '.gitignore', @@ -554,7 +514,7 @@ describe('autofix', () => { autofix({ ...params, eslint: false, prettier: false }), ).resolves.toBeUndefined(); - expectAutofixCommit({ eslint: false, prettier: false }); + expectAutofixCommit({ eslint: false, internal: true }); expect(GitHub.uploadAllFileChanges).toHaveBeenNthCalledWith(1, { branch: 'dev', @@ -567,7 +527,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to push codegen updates... + Attempting to autofix issues (skuba, Prettier)... Pushed fix commit commit-sha. " `); @@ -585,7 +545,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Could not determine the current branch. Please propagate BUILDKITE_BRANCH, GITHUB_HEAD_REF, GITHUB_REF_NAME, or the .git directory to your container. " @@ -605,7 +565,7 @@ describe('autofix', () => { expect(stdout()).toMatchInlineSnapshot(` " - Trying to autofix with ESLint and Prettier... + Attempting to autofix issues (ESLint, skuba, Prettier)... Failed to push fix commit. Does your CI environment have write access to your Git repository? Error: Badness! diff --git a/src/cli/lint/autofix.ts b/src/cli/lint/autofix.ts index a57a47292..f0254e0ad 100644 --- a/src/cli/lint/autofix.ts +++ b/src/cli/lint/autofix.ts @@ -1,7 +1,5 @@ -import path from 'path'; import { inspect } from 'util'; -import fs from 'fs-extra'; import simpleGit from 'simple-git'; import * as Buildkite from '../../api/buildkite'; @@ -12,34 +10,14 @@ import { createLogger, log } from '../../utils/logging'; import { throwOnTimeout } from '../../utils/wait'; import { runESLint } from '../adapter/eslint'; import { runPrettier } from '../adapter/prettier'; -import { RENOVATE_CONFIG_FILENAMES } from '../configure/modules/renovate'; -import { REFRESHABLE_IGNORE_FILES } from '../configure/refreshIgnoreFiles'; +import { internalLint } from './internal'; import type { Input } from './types'; const RENOVATE_DEFAULT_PREFIX = 'renovate'; -// TODO: glob `**/jest.*setup*.ts`? -export const JEST_SETUP_FILES = ['jest.setup.ts', 'jest.setup.int.ts']; - -export const SERVER_LISTENER_FILENAME = 'src/listen.ts'; - const AUTOFIX_COMMIT_MESSAGE = 'Run `skuba format`'; -const AUTOFIX_DELETE_FILES = [ - // Try to delete this SEEK-Jobs/gutenberg automation file that may have been - // accidentally committed in a prior autofix. - 'Dockerfile-incunabulum', -]; - -const AUTOFIX_CODEGEN_FILES = new Set([ - ...AUTOFIX_DELETE_FILES, - ...JEST_SETUP_FILES, - ...REFRESHABLE_IGNORE_FILES, - ...RENOVATE_CONFIG_FILENAMES, - SERVER_LISTENER_FILENAME, -]); - export const AUTOFIX_IGNORE_FILES: Git.ChangedFile[] = [ { path: '.npmrc', @@ -120,51 +98,13 @@ interface AutofixParameters { eslint: boolean; prettier: boolean; + internal: boolean; } -/** - * @returns Whether skuba codegenned a file change which should be included in - * an autofix commit. - */ -const tryCodegen = async (dir: string): Promise => { - try { - // Try to forcibly remove `AUTOFIX_DELETE_FILES` from source control. - // These may include outdated configuration files or internal files that - // were accidentally committed by an autofix. - await Promise.all( - AUTOFIX_DELETE_FILES.map((filename) => - fs.promises.rm(path.join(dir, filename), { force: true }), - ), - ); - - // Search codegenned file changes in the local Git working directory. - // These may include the `AUTOFIX_DELETE_FILES` deleted above or fixups to - // ignore files and module exports that were run at the start of the - // `skuba lint` command. - const changedFiles = await Git.getChangedFiles({ - dir, - - ignore: AUTOFIX_IGNORE_FILES, - }); - - // Determine if a meaningful codegen change - return changedFiles.some((changedFile) => - AUTOFIX_CODEGEN_FILES.has(changedFile.path), - ); - } catch (err) { - log.warn(log.bold('Failed to evaluate codegen changes.')); - log.subtle(inspect(err)); - - return false; - } -}; - export const autofix = async (params: AutofixParameters): Promise => { const dir = process.cwd(); - const codegen = await tryCodegen(dir); - - if (!params.eslint && !params.prettier && !codegen) { + if (!params.eslint && !params.prettier && !params.internal) { return; } @@ -179,25 +119,31 @@ export const autofix = async (params: AutofixParameters): Promise => { try { log.newline(); - if (!params.eslint && !params.prettier) { - log.warn('Trying to push codegen updates...'); - } else { - log.warn( - `Trying to autofix with ${ - params.eslint ? 'ESLint and ' : '' - }Prettier...`, - ); - const logger = createLogger(params.debug); + log.warn( + `Attempting to autofix issues (${[ + params.eslint ? 'ESLint' : undefined, + params.internal ? 'skuba' : undefined, + 'Prettier', // Prettier is always run + ] + .filter((s) => s !== undefined) + .join(', ')})...`, + ); - if (params.eslint) { - await runESLint('format', logger); - } - // Unconditionally re-run Prettier; reaching here means we have pre-existing - // format violations or may have created new ones through ESLint fixes. - await runPrettier('format', logger); + const logger = createLogger(params.debug); + + if (params.internal) { + await internalLint('format'); } + if (params.eslint) { + await runESLint('format', logger); + } + + // Unconditionally re-run Prettier; reaching here means we have pre-existing + // format violations or may have created new ones through ESLint/internal fixes. + await runPrettier('format', logger); + if (process.env.GITHUB_ACTIONS) { // GitHub runners have Git installed locally const ref = await Git.commitAllChanges({ diff --git a/src/cli/lint/external.ts b/src/cli/lint/external.ts index 297ed95ba..03354868f 100644 --- a/src/cli/lint/external.ts +++ b/src/cli/lint/external.ts @@ -5,7 +5,6 @@ import { log } from '../../utils/logging'; import { throwOnTimeout } from '../../utils/wait'; import { createAnnotations } from './annotate'; -import { autofix } from './autofix'; import { runESLintInCurrentThread, runESLintInWorkerThread } from './eslint'; import { runPrettierInCurrentThread, @@ -109,9 +108,8 @@ export const externalLint = async (input: Input) => { process.exitCode = 1; } - await autofix({ - debug: input.debug, - eslint: eslint.fixable, - prettier: !prettier.ok, - }); + return { + eslint, + prettier, + }; }; diff --git a/src/cli/lint/index.ts b/src/cli/lint/index.ts index dda41cf8c..1db48060c 100644 --- a/src/cli/lint/index.ts +++ b/src/cli/lint/index.ts @@ -1,9 +1,8 @@ import type { Writable } from 'stream'; import { hasDebugFlag, hasSerialFlag } from '../../utils/args'; -import { tryRefreshIgnoreFiles } from '../configure/refreshIgnoreFiles'; -import { upgradeSkuba } from '../configure/upgrade'; +import { autofix } from './autofix'; import { externalLint } from './external'; import { internalLint } from './internal'; import type { Input } from './types'; @@ -13,8 +12,6 @@ export const lint = async ( tscOutputStream: Writable | undefined = undefined, workerThreads = true, ) => { - await Promise.all([tryRefreshIgnoreFiles(), upgradeSkuba()]); - const opts: Input = { debug: hasDebugFlag(args), serial: hasSerialFlag(args), @@ -22,7 +19,13 @@ export const lint = async ( workerThreads, }; - await externalLint(opts); + const external = await externalLint(opts); + const internal = await internalLint('lint', opts); - await internalLint(); + await autofix({ + debug: opts.debug, + eslint: external.eslint.fixable, + prettier: !external.prettier.ok, + internal: internal.fixable, + }); }; diff --git a/src/cli/lint/internal.test.ts b/src/cli/lint/internal.test.ts index fa4c801c5..59b986757 100644 --- a/src/cli/lint/internal.test.ts +++ b/src/cli/lint/internal.test.ts @@ -1,6 +1,19 @@ import { internalLint } from './internal'; describe('internalLint', () => { + beforeEach(() => { + jest.spyOn(console, 'log').mockImplementation(() => { + /* no-op */ + }); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + it('passes on skuba itself', () => - expect(internalLint()).resolves.toBeUndefined()); + expect(internalLint('lint')).resolves.toEqual({ + ok: true, + fixable: false, + })); }); diff --git a/src/cli/lint/internal.ts b/src/cli/lint/internal.ts index 8c0c112ed..d4dea4fc9 100644 --- a/src/cli/lint/internal.ts +++ b/src/cli/lint/internal.ts @@ -1,40 +1,95 @@ -import path from 'path'; +import { inspect } from 'util'; import chalk from 'chalk'; -import { pathExists } from 'fs-extra'; -import { log } from '../../utils/logging'; -import { getConsumerManifest } from '../../utils/manifest'; +import { type Logger, createLogger } from '../../utils/logging'; import { detectPackageManager } from '../../utils/packageManager'; -const noSkubaTemplateJs = async () => { - const [manifest, packageManager] = await Promise.all([ - getConsumerManifest(), - detectPackageManager(), - ]); +import { deleteFilesLint } from './internalLints/deleteFiles'; +import { noSkubaTemplateJs } from './internalLints/noSkubaTemplateJs'; +import { tryRefreshIgnoreFiles } from './internalLints/refreshIgnoreFiles'; +import type { Input } from './types'; +import { upgradeSkuba } from './upgrade'; - if (!manifest) { - return; +const lints = [ + deleteFilesLint, + noSkubaTemplateJs, + tryRefreshIgnoreFiles, + upgradeSkuba, +]; + +const lintSerially = async (mode: 'format' | 'lint', logger: Logger) => { + const results = []; + for (const lint of lints) { + results.push(await lint(mode, logger)); } + return results; +}; + +const lintConcurrently = (mode: 'format' | 'lint', logger: Logger) => + Promise.all(lints.map((lint) => lint(mode, logger))); - const templateConfigPath = path.join( - path.dirname(manifest.path), - 'skuba.template.js', +const selectLintFunction = (input?: Input) => { + const isSerial = input?.debug || input?.serial; + return isSerial ? lintSerially : lintConcurrently; +}; + +export const internalLint = async (mode: 'format' | 'lint', input?: Input) => { + const start = process.hrtime.bigint(); + const logger = createLogger( + input?.debug ?? false, + ...(mode === 'lint' ? [chalk.blueBright('skuba │')] : []), ); - if (await pathExists(templateConfigPath)) { - log.err( - `Template is incomplete; run ${chalk.bold( - packageManager.exec, - 'skuba', - 'configure', - )}. ${chalk.dim('no-skuba-template-js')}`, - ); + try { + const lint = selectLintFunction(input); + const results = await lint(mode, logger); + const result = await processResults(results, logger); + const end = process.hrtime.bigint(); + logger.plain(`Processed skuba lints in ${logger.timing(start, end)}.`); + return result; + } catch (err) { + logger.err(logger.bold('Failed to run skuba lints.')); + logger.subtle(inspect(err)); process.exitCode = 1; + + return { ok: false, fixable: false }; } }; -export const internalLint = async () => { - await noSkubaTemplateJs(); +const processResults = async ( + results: Array<{ ok: boolean; fixable: boolean }>, + logger: Logger, +) => { + const result = results.reduce( + (cur, next) => ({ + ok: cur.ok && next.ok, + fixable: cur.fixable || next.fixable, + }), + { ok: true, fixable: false }, + ); + + if (!result.ok) { + process.exitCode = 1; + + if (result.fixable) { + const packageManager = await detectPackageManager(); + logger.err( + `Some skuba lints failed. Try running ${logger.bold( + packageManager.exec, + 'skuba', + 'format', + )} to fix them. ${logger.dim('skuba-lint')}`, + ); + } else { + logger.err( + `Some skuba lints failed which require triage. ${logger.dim( + 'skuba-lint', + )}`, + ); + } + } + + return result; }; diff --git a/src/cli/lint/internalLints/deleteFiles.test.ts b/src/cli/lint/internalLints/deleteFiles.test.ts new file mode 100644 index 000000000..dc8a2052a --- /dev/null +++ b/src/cli/lint/internalLints/deleteFiles.test.ts @@ -0,0 +1,99 @@ +import path from 'path'; + +import * as fsExtra from 'fs-extra'; + +import { log } from '../../../utils/logging'; + +import { deleteFilesLint } from './deleteFiles'; + +const stdoutMock = jest.fn(); + +const stdout = () => stdoutMock.mock.calls.flat(1).join(''); + +jest.mock('fs-extra', () => ({ + pathExists: jest.fn(), + rm: jest.fn(), +})); + +beforeEach(() => { + jest + .spyOn(console, 'log') + .mockImplementation((...args) => stdoutMock(`${args.join(' ')}\n`)); +}); + +afterEach(jest.resetAllMocks); + +describe('deleteFilesLint', () => { + const pathExists = fsExtra.pathExists as jest.MockedFunction< + (path: string) => Promise + >; + const rm = jest.mocked(fsExtra.rm); + + describe('lint mode', () => { + it('should report ok if no files to delete, and output nothing', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(false); + + await expect(deleteFilesLint('lint', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe(''); + + expect(pathExists).toHaveBeenCalledTimes(1); + expect(rm).not.toHaveBeenCalled(); + }); + + it('should report not ok + fixable if files to delete, and output a message', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(true); + + await expect(deleteFilesLint('lint', log)).resolves.toEqual({ + ok: false, + fixable: true, + }); + + expect(stdout()).toBe( + 'Some files should be deleted. Run pnpm exec skuba format to delete them. delete-files\n', + ); + + expect(pathExists).toHaveBeenCalledTimes(1); + expect(rm).not.toHaveBeenCalled(); + }); + }); + + describe('format mode', () => { + it('should report ok if no files to delete, and delete or output nothing', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(false); + + await expect(deleteFilesLint('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe(''); + + expect(pathExists).toHaveBeenCalledTimes(1); + expect(rm).not.toHaveBeenCalled(); + }); + + it('should report ok if files to delete, and delete them and output filenames', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(true); + + await expect(deleteFilesLint('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe('Deleting Dockerfile-incunabulum.\n'); + + expect(pathExists).toHaveBeenCalledTimes(1); + expect(rm).toHaveBeenCalledTimes(1); + expect(rm).toHaveBeenCalledWith( + path.join(process.cwd(), 'Dockerfile-incunabulum'), + { + force: true, + }, + ); + }); + }); +}); diff --git a/src/cli/lint/internalLints/deleteFiles.ts b/src/cli/lint/internalLints/deleteFiles.ts new file mode 100644 index 000000000..e59513231 --- /dev/null +++ b/src/cli/lint/internalLints/deleteFiles.ts @@ -0,0 +1,81 @@ +import path from 'path'; +import { inspect } from 'util'; + +import { pathExists, rm } from 'fs-extra'; + +import type { Logger } from '../../../utils/logging'; +import { detectPackageManager } from '../../../utils/packageManager'; + +const AUTOFIX_DELETE_FILES = [ + // Try to delete this SEEK-Jobs/gutenberg automation file that may have been + // accidentally committed in old versions of skuba. + 'Dockerfile-incunabulum', +]; + +export const deleteFilesLint = async ( + mode: 'format' | 'lint', + logger: Logger, +) => { + const dir = process.cwd(); + + const toDelete = ( + await Promise.all( + AUTOFIX_DELETE_FILES.map( + async (filename) => + [filename, await pathExists(path.join(dir, filename))] as const, + ), + ) + ) + .filter(([, exists]) => exists) + .map(([filename]) => filename); + + if (mode === 'format') { + if (toDelete.length === 0) { + return { ok: true, fixable: false }; + } + + try { + await Promise.all( + toDelete.map((filename) => { + logger.warn(`Deleting ${logger.bold(filename)}.`); + return rm(path.join(dir, filename), { force: true }); + }), + ); + + return { + ok: true, + fixable: false, + }; + } catch (err) { + logger.warn(logger.bold('Failed to delete files.')); + logger.subtle(inspect(err)); + + return { + ok: true, // It's not really a huge deal if we can't delete these files + fixable: false, + }; + } + } + + if (toDelete.length) { + const packageManager = await detectPackageManager(); + + logger.warn( + `Some files should be deleted. Run ${logger.bold( + packageManager.exec, + 'skuba', + 'format', + )} to delete them. ${logger.dim('delete-files')}`, + ); + + return { + ok: false, + fixable: true, + }; + } + + return { + ok: true, + fixable: false, + }; +}; diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts new file mode 100644 index 000000000..7ed4e281d --- /dev/null +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts @@ -0,0 +1,61 @@ +import * as fsExtra from 'fs-extra'; + +import { log } from '../../../utils/logging'; + +import { noSkubaTemplateJs } from './noSkubaTemplateJs'; + +const stdoutMock = jest.fn(); + +const stdout = () => stdoutMock.mock.calls.flat(1).join(''); + +jest.mock('fs-extra', () => ({ + pathExists: jest.fn(), +})); + +beforeEach(() => { + jest + .spyOn(console, 'log') + .mockImplementation((...args) => stdoutMock(`${args.join(' ')}\n`)); +}); + +afterEach(jest.resetAllMocks); + +describe('noSkubaTemplateJs', () => { + const pathExists = fsExtra.pathExists as jest.MockedFunction< + (path: string) => Promise + >; + + describe.each` + mode + ${'format'} + ${'lint'} + `('$mode mode', ({ mode }) => { + it('should report ok if skuba.template.js does not exist, and output nothing', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(false); + + await expect(noSkubaTemplateJs(mode, log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe(''); + + expect(pathExists).toHaveBeenCalledTimes(1); + }); + + it('should report not ok + not fixable if skuba.template.js exists, and output a message', async () => { + jest.mocked(pathExists).mockResolvedValueOnce(true); + + await expect(noSkubaTemplateJs(mode, log)).resolves.toEqual({ + ok: false, + fixable: false, + }); + + expect(stdout()).toBe( + 'Template is incomplete; run pnpm exec skuba configure. no-skuba-template-js\n', + ); + + expect(pathExists).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.ts new file mode 100644 index 000000000..bfe48a05a --- /dev/null +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.ts @@ -0,0 +1,41 @@ +import path from 'path'; + +import { pathExists } from 'fs-extra'; + +import type { Logger } from '../../../utils/logging'; +import { getConsumerManifest } from '../../../utils/manifest'; +import { detectPackageManager } from '../../../utils/packageManager'; + +export const noSkubaTemplateJs = async ( + _mode: 'format' | 'lint', + logger: Logger, +) => { + const [manifest, packageManager] = await Promise.all([ + getConsumerManifest(), + detectPackageManager(), + ]); + + if (!manifest) { + // This will throw elsewhere + return { ok: true, fixable: false }; + } + + const templateConfigPath = path.join( + path.dirname(manifest.path), + 'skuba.template.js', + ); + + if (await pathExists(templateConfigPath)) { + logger.err( + `Template is incomplete; run ${logger.bold( + packageManager.exec, + 'skuba', + 'configure', + )}. ${logger.dim('no-skuba-template-js')}`, + ); + + return { ok: false, fixable: false }; + } + + return { ok: true, fixable: false }; +}; diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts new file mode 100644 index 000000000..5f1789859 --- /dev/null +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts @@ -0,0 +1,143 @@ +import path from 'path'; + +import * as fsExtra from 'fs-extra'; + +import { log } from '../../../utils/logging'; +import * as project from '../../configure/analysis/project'; + +import { refreshIgnoreFiles } from './refreshIgnoreFiles'; + +const stdoutMock = jest.fn(); + +const stdout = () => stdoutMock.mock.calls.flat(1).join(''); + +jest.mock('fs-extra', () => ({ + writeFile: jest.fn(), +})); + +jest.mock('../../../utils/template', () => ({ + readBaseTemplateFile: (name: string) => + Promise.resolve( + `# managed by skuba\nfake content for ${name}\n# end managed by skuba`, + ), +})); + +jest.mock('../../configure/analysis/project'); + +beforeEach(() => { + jest + .spyOn(console, 'log') + .mockImplementation((...args) => stdoutMock(`${args.join(' ')}\n`)); +}); + +afterEach(jest.resetAllMocks); + +describe('refreshIgnoreFiles', () => { + const writeFile = jest.mocked(fsExtra.writeFile); + const createDestinationFileReader = jest.mocked( + project.createDestinationFileReader, + ); + + function setupDestinationFiles(data: Record) { + createDestinationFileReader.mockImplementation( + () => (filename: string) => Promise.resolve(data[filename]), + ); + } + + describe('lint mode', () => { + it('should report ok if files are up to date, and output nothing', async () => { + setupDestinationFiles({ + '.eslintignore': + '# managed by skuba\nfake content for _.eslintignore\n# end managed by skuba', + '.gitignore': + '# managed by skuba\nfake content for _.gitignore\n# end managed by skuba\n\nstuff afterwards', + '.prettierignore': + '# managed by skuba\nfake content for _.prettierignore\n# end managed by skuba', + }); + + await expect(refreshIgnoreFiles('lint', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe(''); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('should report not ok + fixable if files are out of date, and output a message', async () => { + setupDestinationFiles({ + '.eslintignore': + '# managed by skuba\nfake content for _.eslintignore\nextra# end managed by skuba', + '.gitignore': + '# managed by skuba\n# end managed by skuba\n\nstuff afterwards', + '.prettierignore': + '# managed by skuba\nfake content for _.prettierignore\n# end managed by skuba\nstuff afterwards', + }); + + await expect(refreshIgnoreFiles('lint', log)).resolves.toEqual({ + ok: false, + fixable: true, + }); + + expect(`\n${stdout()}`).toBe(` +The .eslintignore file is out of date. Run pnpm exec skuba format to update it. refresh-ignore-files +The .gitignore file is out of date. Run pnpm exec skuba format to update it. refresh-ignore-files +`); + + expect(writeFile).not.toHaveBeenCalled(); + }); + }); + + describe('format mode', () => { + it('should report ok if files are up to date, and write or output nothing', async () => { + setupDestinationFiles({ + '.eslintignore': + '# managed by skuba\nfake content for _.eslintignore\n# end managed by skuba', + '.gitignore': + '# managed by skuba\nfake content for _.gitignore\n# end managed by skuba\n\nstuff afterwards', + '.prettierignore': + '# managed by skuba\nfake content for _.prettierignore\n# end managed by skuba', + }); + + await expect(refreshIgnoreFiles('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe(''); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('should report ok if files are out of date, and update them and output filenames', async () => { + setupDestinationFiles({ + '.eslintignore': + '# managed by skuba\nfake content for _.eslintignore\nextra# end managed by skuba', + '.gitignore': + '# managed by skuba\n# end managed by skuba\n\nstuff afterwards', + '.prettierignore': + '# managed by skuba\nfake content for _.prettierignore\n# end managed by skuba\nstuff afterwards', + }); + + await expect(refreshIgnoreFiles('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(stdout()).toBe( + 'Refreshed .eslintignore. refresh-ignore-files\nRefreshed .gitignore. refresh-ignore-files\n', + ); + + expect(writeFile).toHaveBeenCalledTimes(2); + expect(writeFile).toHaveBeenCalledWith( + path.join(process.cwd(), '.eslintignore'), + '# managed by skuba\nfake content for _.eslintignore\n# end managed by skuba', + ); + expect(writeFile).toHaveBeenCalledWith( + path.join(process.cwd(), '.gitignore'), + '# managed by skuba\nfake content for _.gitignore\n# end managed by skuba\n\nstuff afterwards', + ); + }); + }); +}); diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.ts new file mode 100644 index 000000000..64890da3a --- /dev/null +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.ts @@ -0,0 +1,113 @@ +import path from 'path'; +import { inspect } from 'util'; + +import { writeFile } from 'fs-extra'; + +import type { Logger } from '../../../utils/logging'; +import { detectPackageManager } from '../../../utils/packageManager'; +import { readBaseTemplateFile } from '../../../utils/template'; +import { getDestinationManifest } from '../../configure/analysis/package'; +import { createDestinationFileReader } from '../../configure/analysis/project'; +import { mergeWithIgnoreFile } from '../../configure/processing/ignoreFile'; + +const REFRESHABLE_IGNORE_FILES = [ + '.eslintignore', + '.gitignore', + '.prettierignore', +]; + +export const refreshIgnoreFiles = async ( + mode: 'format' | 'lint', + logger: Logger, +) => { + // TODO: check current state of .gitignore + // If it contains !.npmrc, break + // If it contains .npmrc, we can either + // 1. Move the entry below the skuba-managed section for manual triage + // 2. Delete any local .npmrc state before un-ignoring the .npmrc + + const manifest = await getDestinationManifest(); + + const destinationRoot = path.dirname(manifest.path); + + const readDestinationFile = createDestinationFileReader(destinationRoot); + + const refreshIgnoreFile = async (filename: string) => { + const [inputFile, templateFile] = await Promise.all([ + readDestinationFile(filename), + readBaseTemplateFile(`_${filename}`), + ]); + + const data = inputFile + ? mergeWithIgnoreFile(templateFile)(inputFile) + : templateFile; + + const filepath = path.join(destinationRoot, filename); + + if (mode === 'format') { + if (data === inputFile) { + return { needsChange: false }; + } + + await writeFile(filepath, data); + return { + needsChange: false, + msg: `Refreshed ${logger.bold(filename)}. ${logger.dim( + 'refresh-ignore-files', + )}`, + }; + } + + if (data !== inputFile) { + const packageManager = await detectPackageManager(); + + return { + needsChange: true, + msg: `The ${logger.bold( + filename, + )} file is out of date. Run ${logger.bold( + packageManager.exec, + 'skuba', + 'format', + )} to update it. ${logger.dim('refresh-ignore-files')}`, + }; + } + + return { needsChange: false }; + }; + + const results = await Promise.all( + REFRESHABLE_IGNORE_FILES.map(refreshIgnoreFile), + ); + + // Log after for reproducible test output ordering + results.forEach((result) => { + if (result.msg) { + logger.warn(result.msg); + } + }); + + const anyNeedChanging = results.some(({ needsChange }) => needsChange); + + return { + ok: !anyNeedChanging, + fixable: anyNeedChanging, + }; +}; + +export const tryRefreshIgnoreFiles = async ( + mode: 'format' | 'lint', + logger: Logger, +) => { + try { + return await refreshIgnoreFiles(mode, logger); + } catch (err) { + logger.warn('Failed to refresh ignore files.'); + logger.subtle(inspect(err)); + + return { + ok: false, + fixable: false, + }; + } +}; diff --git a/src/cli/configure/upgrade/index.test.ts b/src/cli/lint/upgrade/index.test.ts similarity index 57% rename from src/cli/configure/upgrade/index.test.ts rename to src/cli/lint/upgrade/index.test.ts index 671ec2948..b5274aef8 100644 --- a/src/cli/configure/upgrade/index.test.ts +++ b/src/cli/lint/upgrade/index.test.ts @@ -1,6 +1,7 @@ import { readdir, writeFile } from 'fs-extra'; import type { NormalizedPackageJson } from 'read-pkg-up'; +import { log } from '../../../utils/logging'; import { getConsumerManifest } from '../../../utils/manifest'; import { getSkubaVersion } from '../../../utils/version'; @@ -15,11 +16,11 @@ beforeEach(() => { jest.clearAllMocks(); }); -describe('upgradeSkuba', () => { +describe('upgradeSkuba in format mode', () => { it('should throw an error if no skuba manifest can be found', async () => { jest.mocked(getConsumerManifest).mockResolvedValue(undefined); - await expect(upgradeSkuba()).rejects.toThrow( + await expect(upgradeSkuba('format', log)).rejects.toThrow( 'Could not find a package json for this project', ); }); @@ -40,7 +41,10 @@ describe('upgradeSkuba', () => { jest.mocked(getSkubaVersion).mockResolvedValue('2.0.0'); - await expect(upgradeSkuba()).resolves.toBeUndefined(); + await expect(upgradeSkuba('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); expect(readdir).not.toHaveBeenCalled(); }); @@ -80,7 +84,10 @@ describe('upgradeSkuba', () => { .mocked(readdir) .mockResolvedValue(['0.9.0', '1.0.0', '2.0.0'] as never); - await expect(upgradeSkuba()).resolves.toBeUndefined(); + await expect(upgradeSkuba('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); expect(mockUpgrade.upgrade).toHaveBeenCalledTimes(2); }); @@ -111,7 +118,10 @@ describe('upgradeSkuba', () => { // readdir has overloads and the mocked version doesn't match the string version jest.mocked(readdir).mockResolvedValue(['2.0.0'] as never); - await expect(upgradeSkuba()).resolves.toBeUndefined(); + await expect(upgradeSkuba('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); expect(writeFile).toHaveBeenCalledWith( '/package.json', @@ -150,7 +160,10 @@ describe('upgradeSkuba', () => { // readdir has overloads and the mocked version doesn't match the string version jest.mocked(readdir).mockResolvedValue(['2.0.0'] as never); - await expect(upgradeSkuba()).resolves.toBeUndefined(); + await expect(upgradeSkuba('format', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); expect(writeFile).toHaveBeenCalledWith( '/package.json', @@ -165,3 +178,88 @@ describe('upgradeSkuba', () => { ); }); }); + +describe('upgradeSkuba in lint mode', () => { + it('should throw an error if no skuba manifest can be found', async () => { + jest.mocked(getConsumerManifest).mockResolvedValue(undefined); + + await expect(upgradeSkuba('lint', log)).rejects.toThrow( + 'Could not find a package json for this project', + ); + }); + + it('should return early if the skuba manifest version is greater than or equal to the skuba current version', async () => { + jest.mocked(getConsumerManifest).mockResolvedValue({ + packageJson: { + skuba: { + version: '2.0.0', + }, + _id: 'test', + name: 'some-api', + readme: '', + version: '1.0.0', + } as NormalizedPackageJson, + path: '/package.json', + }); + + jest.mocked(getSkubaVersion).mockResolvedValue('2.0.0'); + + await expect(upgradeSkuba('lint', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + + expect(readdir).not.toHaveBeenCalled(); + }); + + it('should return ok: false, fixable: true if there are lints to apply', async () => { + jest.mocked(getConsumerManifest).mockResolvedValue({ + packageJson: { + skuba: { + version: '1.0.0', + }, + _id: 'test', + name: 'some-api', + readme: '', + version: '1.0.0', + } as NormalizedPackageJson, + path: '/package.json', + }); + + jest.mocked(getSkubaVersion).mockResolvedValue('2.0.0'); + + // readdir has overloads and the mocked version doesn't match the string version + jest + .mocked(readdir) + .mockResolvedValue(['0.9.0', '1.0.0', '2.0.0'] as never); + + await expect(upgradeSkuba('lint', log)).resolves.toEqual({ + ok: false, + fixable: true, + }); + }); + + it('should return ok: true, fixable: false if there are no lints to apply despite package.json being out of date', async () => { + jest.mocked(getConsumerManifest).mockResolvedValue({ + packageJson: { + skuba: { + version: '1.0.0', + }, + _id: 'test', + name: 'some-api', + readme: '', + version: '1.0.0', + } as NormalizedPackageJson, + path: '/package.json', + }); + + jest.mocked(getSkubaVersion).mockResolvedValue('2.0.0'); + + jest.mocked(readdir).mockResolvedValue(['0.9.0'] as never); + + await expect(upgradeSkuba('lint', log)).resolves.toEqual({ + ok: true, + fixable: false, + }); + }); +}); diff --git a/src/cli/configure/upgrade/index.ts b/src/cli/lint/upgrade/index.ts similarity index 60% rename from src/cli/configure/upgrade/index.ts rename to src/cli/lint/upgrade/index.ts index acf6b48eb..ef7b77643 100644 --- a/src/cli/configure/upgrade/index.ts +++ b/src/cli/lint/upgrade/index.ts @@ -3,11 +3,12 @@ import path from 'path'; import { readdir, writeFile } from 'fs-extra'; import { gte, sort } from 'semver'; -import { log } from '../../../utils/logging'; +import type { Logger } from '../../../utils/logging'; import { getConsumerManifest } from '../../../utils/manifest'; +import { detectPackageManager } from '../../../utils/packageManager'; import { getSkubaVersion } from '../../../utils/version'; +import { formatPackage } from '../../configure/processing/package'; import type { SkubaPackageJson } from '../../init/writePackageJson'; -import { formatPackage } from '../processing/package'; const getPatches = async (manifestVersion: string): Promise => { const patches = await readdir(path.join(__dirname, 'patches')); @@ -37,7 +38,7 @@ const resolvePatch = async (patch: string): Promise => { throw new Error(`Could not resolve patch ${patch}`); }; -export const upgradeSkuba = async () => { +export const upgradeSkuba = async (mode: 'lint' | 'format', logger: Logger) => { const [currentVersion, manifest] = await Promise.all([ getSkubaVersion(), getConsumerManifest(), @@ -52,21 +53,45 @@ export const upgradeSkuba = async () => { const manifestVersion = (manifest.packageJson.skuba as SkubaPackageJson) .version; - // We are up to date, avoid apply patches + // We are up to date, skip patches if (gte(manifestVersion, currentVersion)) { - return; + return { ok: true, fixable: false }; } - log.plain('Updating skuba...'); - const patches = await getPatches(manifestVersion); + if (patches.length === 0) { + // TODO: Do we want to _always_ write the version? + // Or only if there's associated patches for that version / it's missing? + // Also see 'should return ok: true, fixable: false if there are no lints to apply despite package.json being out of date' + return { ok: true, fixable: false }; + } + + if (mode === 'lint') { + const packageManager = await detectPackageManager(); + + logger.warn( + `skuba has patches to apply. Run ${logger.bold( + packageManager.exec, + 'skuba', + 'format', + )} to run them. ${logger.dim('skuba-patches')}`, + ); + + // TODO: Do we want to declare patches as optional? + // TODO: Should we return ok: true if all patches are no-ops? + + return { ok: false, fixable: true }; + } + + logger.plain('Updating skuba...'); + // Run these in series in case a subsequent patch relies on a previous patch for (const patch of patches) { const patchFile = await resolvePatch(patch); await patchFile.upgrade(); - log.newline(); - log.plain(`Patch ${patch} applied.`); + logger.newline(); + logger.plain(`Patch ${patch} applied.`); } (manifest.packageJson.skuba as SkubaPackageJson).version = currentVersion; @@ -74,7 +99,12 @@ export const upgradeSkuba = async () => { const updatedPackageJson = await formatPackage(manifest.packageJson); await writeFile(manifest.path, updatedPackageJson); - log.newline(); - log.plain('skuba update complete.'); - log.newline(); + logger.newline(); + logger.plain('skuba update complete.'); + logger.newline(); + + return { + ok: true, + fixable: false, + }; }; diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts b/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.test.ts similarity index 94% rename from src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts rename to src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.test.ts index 6c4ee900a..170acb186 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts +++ b/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.test.ts @@ -1,7 +1,7 @@ import fs from 'fs-extra'; -import * as packageAnalysis from '../../../analysis/package'; -import * as projectAnalysis from '../../../analysis/project'; +import * as packageAnalysis from '../../../../configure/analysis/package'; +import * as projectAnalysis from '../../../../configure/analysis/project'; import { tryAddEmptyExports } from './addEmptyExports'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts b/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.ts similarity index 82% rename from src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts rename to src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.ts index a57373369..d4b1f445d 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts +++ b/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.ts @@ -4,10 +4,11 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { JEST_SETUP_FILES } from '../../../../lint/autofix'; -import { getDestinationManifest } from '../../../analysis/package'; -import { createDestinationFileReader } from '../../../analysis/project'; -import { formatPrettier } from '../../../processing/prettier'; +import { getDestinationManifest } from '../../../../configure/analysis/package'; +import { createDestinationFileReader } from '../../../../configure/analysis/project'; +import { formatPrettier } from '../../../../configure/processing/prettier'; + +const JEST_SETUP_FILES = ['jest.setup.ts', 'jest.setup.int.ts']; const addEmptyExports = async () => { const manifest = await getDestinationManifest(); diff --git a/src/cli/configure/upgrade/patches/7.3.1/index.ts b/src/cli/lint/upgrade/patches/7.3.1/index.ts similarity index 80% rename from src/cli/configure/upgrade/patches/7.3.1/index.ts rename to src/cli/lint/upgrade/patches/7.3.1/index.ts index 314ef56f8..984522657 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/index.ts +++ b/src/cli/lint/upgrade/patches/7.3.1/index.ts @@ -1,4 +1,4 @@ -import { tryPatchRenovateConfig } from '../../../patchRenovateConfig'; +import { tryPatchRenovateConfig } from '../../../../configure/patchRenovateConfig'; import { tryAddEmptyExports } from './addEmptyExports'; import { tryPatchDockerfile } from './patchDockerfile'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts b/src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.test.ts similarity index 100% rename from src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts rename to src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.test.ts diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts b/src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.ts similarity index 91% rename from src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts rename to src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.ts index 6a265edf2..20ddcdd02 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts +++ b/src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.ts @@ -3,7 +3,7 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { createDestinationFileReader } from '../../../analysis/project'; +import { createDestinationFileReader } from '../../../../configure/analysis/project'; const DOCKERFILE_FILENAME = 'Dockerfile'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts b/src/cli/lint/upgrade/patches/7.3.1/patchServerListener.test.ts similarity index 100% rename from src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts rename to src/cli/lint/upgrade/patches/7.3.1/patchServerListener.test.ts diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts b/src/cli/lint/upgrade/patches/7.3.1/patchServerListener.ts similarity index 85% rename from src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts rename to src/cli/lint/upgrade/patches/7.3.1/patchServerListener.ts index 79bb4570b..e2d4c9667 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts +++ b/src/cli/lint/upgrade/patches/7.3.1/patchServerListener.ts @@ -3,11 +3,12 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { SERVER_LISTENER_FILENAME } from '../../../../lint/autofix'; -import { createDestinationFileReader } from '../../../analysis/project'; -import { formatPrettier } from '../../../processing/prettier'; +import { createDestinationFileReader } from '../../../../configure/analysis/project'; +import { formatPrettier } from '../../../../configure/processing/prettier'; -export const KEEP_ALIVE_CODE = ` +const SERVER_LISTENER_FILENAME = 'src/listen.ts'; + +const KEEP_ALIVE_CODE = ` // Gantry ALB default idle timeout is 30 seconds // https://nodejs.org/docs/latest-v18.x/api/http.html#serverkeepalivetimeout // Node default is 5 seconds diff --git a/src/utils/logging.ts b/src/utils/logging.ts index 8acaca895..5d22263fb 100644 --- a/src/utils/logging.ts +++ b/src/utils/logging.ts @@ -9,6 +9,7 @@ export const createLogger = (debug: boolean, ...prefixes: unknown[]) => { return { bold: chalk.bold, + dim: chalk.dim, formatSubtle: chalk.grey, timing: (start: bigint, end: bigint) => From 50903cfe1384e58e2549e9d29bad21bc7a3fa53b Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Sun, 7 Jan 2024 21:09:15 +1100 Subject: [PATCH 02/13] Move back upgrade to configure to reduce PR diff size --- src/cli/{lint => configure}/upgrade/index.test.ts | 0 src/cli/{lint => configure}/upgrade/index.ts | 0 .../upgrade/patches/7.3.1/addEmptyExports.test.ts | 0 .../upgrade/patches/7.3.1/addEmptyExports.ts | 0 src/cli/{lint => configure}/upgrade/patches/7.3.1/index.ts | 0 .../upgrade/patches/7.3.1/patchDockerfile.test.ts | 0 .../upgrade/patches/7.3.1/patchDockerfile.ts | 0 .../upgrade/patches/7.3.1/patchServerListener.test.ts | 0 .../upgrade/patches/7.3.1/patchServerListener.ts | 0 src/cli/lint/internal.ts | 2 +- 10 files changed, 1 insertion(+), 1 deletion(-) rename src/cli/{lint => configure}/upgrade/index.test.ts (100%) rename src/cli/{lint => configure}/upgrade/index.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/addEmptyExports.test.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/addEmptyExports.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/index.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/patchDockerfile.test.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/patchDockerfile.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/patchServerListener.test.ts (100%) rename src/cli/{lint => configure}/upgrade/patches/7.3.1/patchServerListener.ts (100%) diff --git a/src/cli/lint/upgrade/index.test.ts b/src/cli/configure/upgrade/index.test.ts similarity index 100% rename from src/cli/lint/upgrade/index.test.ts rename to src/cli/configure/upgrade/index.test.ts diff --git a/src/cli/lint/upgrade/index.ts b/src/cli/configure/upgrade/index.ts similarity index 100% rename from src/cli/lint/upgrade/index.ts rename to src/cli/configure/upgrade/index.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.test.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.test.ts rename to src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/addEmptyExports.ts rename to src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/index.ts b/src/cli/configure/upgrade/patches/7.3.1/index.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/index.ts rename to src/cli/configure/upgrade/patches/7.3.1/index.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.test.ts b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.test.ts rename to src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.ts b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/patchDockerfile.ts rename to src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/patchServerListener.test.ts b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/patchServerListener.test.ts rename to src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts diff --git a/src/cli/lint/upgrade/patches/7.3.1/patchServerListener.ts b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts similarity index 100% rename from src/cli/lint/upgrade/patches/7.3.1/patchServerListener.ts rename to src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts diff --git a/src/cli/lint/internal.ts b/src/cli/lint/internal.ts index d4dea4fc9..5724d506e 100644 --- a/src/cli/lint/internal.ts +++ b/src/cli/lint/internal.ts @@ -4,12 +4,12 @@ import chalk from 'chalk'; import { type Logger, createLogger } from '../../utils/logging'; import { detectPackageManager } from '../../utils/packageManager'; +import { upgradeSkuba } from '../configure/upgrade'; import { deleteFilesLint } from './internalLints/deleteFiles'; import { noSkubaTemplateJs } from './internalLints/noSkubaTemplateJs'; import { tryRefreshIgnoreFiles } from './internalLints/refreshIgnoreFiles'; import type { Input } from './types'; -import { upgradeSkuba } from './upgrade'; const lints = [ deleteFilesLint, From 0063ea676b1bf19b8b97356784b71b1c38b93a6d Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Sun, 7 Jan 2024 21:11:16 +1100 Subject: [PATCH 03/13] Put imports back --- src/cli/configure/upgrade/index.ts | 2 +- .../configure/upgrade/patches/7.3.1/addEmptyExports.test.ts | 4 ++-- src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts | 6 +++--- src/cli/configure/upgrade/patches/7.3.1/index.ts | 2 +- src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts | 2 +- .../configure/upgrade/patches/7.3.1/patchServerListener.ts | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/cli/configure/upgrade/index.ts b/src/cli/configure/upgrade/index.ts index ef7b77643..6576430b7 100644 --- a/src/cli/configure/upgrade/index.ts +++ b/src/cli/configure/upgrade/index.ts @@ -7,8 +7,8 @@ import type { Logger } from '../../../utils/logging'; import { getConsumerManifest } from '../../../utils/manifest'; import { detectPackageManager } from '../../../utils/packageManager'; import { getSkubaVersion } from '../../../utils/version'; -import { formatPackage } from '../../configure/processing/package'; import type { SkubaPackageJson } from '../../init/writePackageJson'; +import { formatPackage } from '../processing/package'; const getPatches = async (manifestVersion: string): Promise => { const patches = await readdir(path.join(__dirname, 'patches')); diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts index 170acb186..6c4ee900a 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts @@ -1,7 +1,7 @@ import fs from 'fs-extra'; -import * as packageAnalysis from '../../../../configure/analysis/package'; -import * as projectAnalysis from '../../../../configure/analysis/project'; +import * as packageAnalysis from '../../../analysis/package'; +import * as projectAnalysis from '../../../analysis/project'; import { tryAddEmptyExports } from './addEmptyExports'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts index d4b1f445d..12c897b05 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts @@ -4,9 +4,9 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { getDestinationManifest } from '../../../../configure/analysis/package'; -import { createDestinationFileReader } from '../../../../configure/analysis/project'; -import { formatPrettier } from '../../../../configure/processing/prettier'; +import { getDestinationManifest } from '../../../analysis/package'; +import { createDestinationFileReader } from '../../../analysis/project'; +import { formatPrettier } from '../../../processing/prettier'; const JEST_SETUP_FILES = ['jest.setup.ts', 'jest.setup.int.ts']; diff --git a/src/cli/configure/upgrade/patches/7.3.1/index.ts b/src/cli/configure/upgrade/patches/7.3.1/index.ts index 984522657..314ef56f8 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/index.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/index.ts @@ -1,4 +1,4 @@ -import { tryPatchRenovateConfig } from '../../../../configure/patchRenovateConfig'; +import { tryPatchRenovateConfig } from '../../../patchRenovateConfig'; import { tryAddEmptyExports } from './addEmptyExports'; import { tryPatchDockerfile } from './patchDockerfile'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts index 20ddcdd02..6a265edf2 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts @@ -3,7 +3,7 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { createDestinationFileReader } from '../../../../configure/analysis/project'; +import { createDestinationFileReader } from '../../../analysis/project'; const DOCKERFILE_FILENAME = 'Dockerfile'; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts index e2d4c9667..8491f19a4 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts @@ -3,8 +3,8 @@ import { inspect } from 'util'; import fs from 'fs-extra'; import { log } from '../../../../../utils/logging'; -import { createDestinationFileReader } from '../../../../configure/analysis/project'; -import { formatPrettier } from '../../../../configure/processing/prettier'; +import { createDestinationFileReader } from '../../../analysis/project'; +import { formatPrettier } from '../../../processing/prettier'; const SERVER_LISTENER_FILENAME = 'src/listen.ts'; From 305b8b6cc732e2aaa0a773e12c869009092b152c Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Sun, 7 Jan 2024 22:10:35 +1100 Subject: [PATCH 04/13] Include internal lints in annotations --- src/cli/__snapshots__/lint.int.test.ts.snap | 42 +++++++----- src/cli/configure/upgrade/index.test.ts | 7 ++ src/cli/configure/upgrade/index.ts | 25 ++++++-- src/cli/lint/annotate/buildkite/index.ts | 8 ++- src/cli/lint/annotate/buildkite/internal.ts | 16 +++++ src/cli/lint/annotate/github/index.test.ts | 15 +++++ src/cli/lint/annotate/github/index.ts | 6 +- src/cli/lint/annotate/github/internal.ts | 14 ++++ src/cli/lint/annotate/index.ts | 12 +++- src/cli/lint/external.ts | 30 +-------- src/cli/lint/index.ts | 50 +++++++++++++-- src/cli/lint/internal.test.ts | 1 + src/cli/lint/internal.ts | 64 +++++++------------ .../lint/internalLints/deleteFiles.test.ts | 6 ++ src/cli/lint/internalLints/deleteFiles.ts | 7 +- .../internalLints/noSkubaTemplateJs.test.ts | 6 ++ .../lint/internalLints/noSkubaTemplateJs.ts | 14 +++- .../internalLints/refreshIgnoreFiles.test.ts | 15 +++++ .../lint/internalLints/refreshIgnoreFiles.ts | 19 +++++- 19 files changed, 253 insertions(+), 104 deletions(-) create mode 100644 src/cli/lint/annotate/buildkite/internal.ts create mode 100644 src/cli/lint/annotate/github/internal.ts diff --git a/src/cli/__snapshots__/lint.int.test.ts.snap b/src/cli/__snapshots__/lint.int.test.ts.snap index e078ccc9d..6affc0f02 100644 --- a/src/cli/__snapshots__/lint.int.test.ts.snap +++ b/src/cli/__snapshots__/lint.int.test.ts.snap @@ -24,20 +24,17 @@ Prettier │ d.js Prettier │ package.json tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 0 - -ESLint, Prettier found issues that require triage. skuba │ Processed skuba lints in s. +ESLint, Prettier found issues that require triage. + +Try running yarn skuba format to fix them. " `; exports[`fixable 2`] = ` [ " -Options: { - context: 'skuba-lint-external', - scopeContextToStep: true, - style: 'error' -} +Options: { context: 'skuba-lint', scopeContextToStep: true, style: 'error' } \`skuba lint\` found issues that require triage: @@ -76,12 +73,28 @@ Prettier │ Processed 6 files in s. tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 0 skuba │ skuba has patches to apply. Run yarn skuba format to run them. skuba-patches -skuba │ Some skuba lints failed. Try running yarn skuba format to fix them. skuba-lint skuba │ Processed skuba lints in s. +skuba found issues that require triage. + +Try running yarn skuba format to fix them. " `; -exports[`needs patches 2`] = `[]`; +exports[`needs patches 2`] = ` +[ + " +Options: { context: 'skuba-lint', scopeContextToStep: true, style: 'error' } + +\`skuba lint\` found issues that require triage: + +**skuba** + +\`\`\`term +/package.json skuba has patches to apply. Run yarn skuba format to run them. +\`\`\` +", +] +`; exports[`ok --debug 1`] = ` " @@ -186,20 +199,17 @@ Prettier │ a/a/a.ts tsc │ d.js(1,1): error TS1128: Declaration or statement expected. tsc │ TSFILE: /lib/tsconfig.tsbuildinfo tsc │ tsc --noEmit exited with code 2 - -ESLint, Prettier, tsc found issues that require triage. skuba │ Processed skuba lints in s. +ESLint, Prettier, tsc found issues that require triage. + +Try running yarn skuba format to fix them. " `; exports[`unfixable 2`] = ` [ " -Options: { - context: 'skuba-lint-external', - scopeContextToStep: true, - style: 'error' -} +Options: { context: 'skuba-lint', scopeContextToStep: true, style: 'error' } \`skuba lint\` found issues that require triage: diff --git a/src/cli/configure/upgrade/index.test.ts b/src/cli/configure/upgrade/index.test.ts index b5274aef8..aae125057 100644 --- a/src/cli/configure/upgrade/index.test.ts +++ b/src/cli/configure/upgrade/index.test.ts @@ -236,6 +236,13 @@ describe('upgradeSkuba in lint mode', () => { await expect(upgradeSkuba('lint', log)).resolves.toEqual({ ok: false, fixable: true, + annotations: [ + { + message: + 'skuba has patches to apply. Run pnpm exec skuba format to run them.', + path: '/package.json', + }, + ], }); }); diff --git a/src/cli/configure/upgrade/index.ts b/src/cli/configure/upgrade/index.ts index 6576430b7..a7aab337d 100644 --- a/src/cli/configure/upgrade/index.ts +++ b/src/cli/configure/upgrade/index.ts @@ -8,6 +8,7 @@ import { getConsumerManifest } from '../../../utils/manifest'; import { detectPackageManager } from '../../../utils/packageManager'; import { getSkubaVersion } from '../../../utils/version'; import type { SkubaPackageJson } from '../../init/writePackageJson'; +import type { InternalLintResult } from '../../lint/internal'; import { formatPackage } from '../processing/package'; const getPatches = async (manifestVersion: string): Promise => { @@ -38,7 +39,10 @@ const resolvePatch = async (patch: string): Promise => { throw new Error(`Could not resolve patch ${patch}`); }; -export const upgradeSkuba = async (mode: 'lint' | 'format', logger: Logger) => { +export const upgradeSkuba = async ( + mode: 'lint' | 'format', + logger: Logger, +): Promise => { const [currentVersion, manifest] = await Promise.all([ getSkubaVersion(), getConsumerManifest(), @@ -68,6 +72,9 @@ export const upgradeSkuba = async (mode: 'lint' | 'format', logger: Logger) => { } if (mode === 'lint') { + // TODO: Do we want to declare patches as optional? + // TODO: Should we return ok: true if all patches are no-ops? + const packageManager = await detectPackageManager(); logger.warn( @@ -78,10 +85,18 @@ export const upgradeSkuba = async (mode: 'lint' | 'format', logger: Logger) => { )} to run them. ${logger.dim('skuba-patches')}`, ); - // TODO: Do we want to declare patches as optional? - // TODO: Should we return ok: true if all patches are no-ops? - - return { ok: false, fixable: true }; + return { + ok: false, + fixable: true, + annotations: [ + { + // package.json as likely skuba version has changed + // TODO: location the "skuba": {} config in the package.json and annotate on the version property + path: manifest.path, + message: `skuba has patches to apply. Run ${packageManager.exec} skuba format to run them.`, + }, + ], + }; } logger.plain('Updating skuba...'); diff --git a/src/cli/lint/annotate/buildkite/index.ts b/src/cli/lint/annotate/buildkite/index.ts index 44a39ec37..decd819c4 100644 --- a/src/cli/lint/annotate/buildkite/index.ts +++ b/src/cli/lint/annotate/buildkite/index.ts @@ -2,30 +2,34 @@ import * as Buildkite from '../../../../api/buildkite'; import type { ESLintOutput } from '../../../adapter/eslint'; import type { PrettierOutput } from '../../../adapter/prettier'; import type { StreamInterceptor } from '../../../lint/external'; +import type { InternalLintResult } from '../../internal'; import { createEslintAnnotations } from './eslint'; +import { createInternalAnnotations } from './internal'; import { createPrettierAnnotations } from './prettier'; import { createTscAnnotations } from './tsc'; export const createBuildkiteAnnotations = async ( + internal: InternalLintResult, eslint: ESLintOutput, prettier: PrettierOutput, tscOk: boolean, tscOutputStream: StreamInterceptor, ): Promise => { - if (eslint.ok && prettier.ok && tscOk) { + if (internal.ok && eslint.ok && prettier.ok && tscOk) { return; } const buildkiteOutput = [ '`skuba lint` found issues that require triage:', + ...createInternalAnnotations(internal), ...createEslintAnnotations(eslint), ...createPrettierAnnotations(prettier), ...createTscAnnotations(tscOk, tscOutputStream), ].join('\n\n'); await Buildkite.annotate(buildkiteOutput, { - context: 'skuba-lint-external', + context: 'skuba-lint', scopeContextToStep: true, style: 'error', }); diff --git a/src/cli/lint/annotate/buildkite/internal.ts b/src/cli/lint/annotate/buildkite/internal.ts new file mode 100644 index 000000000..9000f7950 --- /dev/null +++ b/src/cli/lint/annotate/buildkite/internal.ts @@ -0,0 +1,16 @@ +import * as Buildkite from '../../../../api/buildkite'; +import type { InternalLintResult } from '../../internal'; + +export const createInternalAnnotations = ( + internal: InternalLintResult, +): string[] => + !internal.ok && internal.annotations?.length + ? [ + '**skuba**', + Buildkite.md.terminal( + internal.annotations + .map(({ message, path }) => `${path} ${message}`) + .join('\n'), + ), + ] + : []; diff --git a/src/cli/lint/annotate/github/index.test.ts b/src/cli/lint/annotate/github/index.test.ts index 0a36dd9c1..a526d860a 100644 --- a/src/cli/lint/annotate/github/index.test.ts +++ b/src/cli/lint/annotate/github/index.test.ts @@ -2,6 +2,7 @@ import * as GitHub from '../../../../api/github'; import type { ESLintOutput } from '../../../adapter/eslint'; import type { PrettierOutput } from '../../../adapter/prettier'; import type { StreamInterceptor } from '../../../lint/external'; +import type { InternalLintResult } from '../../internal'; import { createEslintAnnotations } from './eslint'; import { createPrettierAnnotations } from './prettier'; @@ -52,6 +53,11 @@ const prettierOutput: PrettierOutput = { }, }; +const internalOutput: InternalLintResult = { + ok: false, + fixable: false, +}; + const tscOk = false; const mockOutput = jest.fn(); @@ -124,6 +130,7 @@ it('should return immediately if the required environment variables are not set' delete process.env.GITHUB_ACTIONS; await createGitHubAnnotations( + internalOutput, eslintOutput, prettierOutput, tscOk, @@ -135,6 +142,7 @@ it('should return immediately if the required environment variables are not set' it('should call createEslintAnnotations with the ESLint output', async () => { await createGitHubAnnotations( + internalOutput, eslintOutput, prettierOutput, tscOk, @@ -146,6 +154,7 @@ it('should call createEslintAnnotations with the ESLint output', async () => { it('should call createPrettierAnnotations with the Prettier output', async () => { await createGitHubAnnotations( + internalOutput, eslintOutput, prettierOutput, tscOk, @@ -157,6 +166,7 @@ it('should call createPrettierAnnotations with the Prettier output', async () => it('should call createTscAnnotations with tscOk and tscOutputStream', async () => { await createGitHubAnnotations( + internalOutput, eslintOutput, prettierOutput, tscOk, @@ -174,6 +184,7 @@ it('should combine all the annotations into an array for the check run', async ( ]; await createGitHubAnnotations( + internalOutput, eslintOutput, prettierOutput, tscOk, @@ -191,6 +202,7 @@ it('should combine all the annotations into an array for the check run', async ( it('should set the conclusion to failure if any output is not ok', async () => { await createGitHubAnnotations( + { ...internalOutput, ok: true }, { ...eslintOutput, ok: false }, { ...prettierOutput, ok: true }, true, @@ -208,6 +220,7 @@ it('should set the conclusion to failure if any output is not ok', async () => { it('should set the conclusion to success if all outputs are ok', async () => { await createGitHubAnnotations( + { ...internalOutput, ok: true }, { ...eslintOutput, ok: true }, { ...prettierOutput, ok: true }, true, @@ -227,6 +240,7 @@ it('should report that skuba lint failed if the output is not ok', async () => { const expectedSummary = '`skuba lint` found issues that require triage.'; await createGitHubAnnotations( + internalOutput, { ...eslintOutput, ok: false }, { ...prettierOutput, ok: false }, false, @@ -246,6 +260,7 @@ it('should set the summary to `Lint passed` if all outputs are ok', async () => const expectedSummary = '`skuba lint` passed.'; await createGitHubAnnotations( + { ...internalOutput, ok: true }, { ...eslintOutput, ok: true }, { ...prettierOutput, ok: true }, true, diff --git a/src/cli/lint/annotate/github/index.ts b/src/cli/lint/annotate/github/index.ts index afdd48ba7..1adaf2e9a 100644 --- a/src/cli/lint/annotate/github/index.ts +++ b/src/cli/lint/annotate/github/index.ts @@ -6,12 +6,15 @@ import { import type { ESLintOutput } from '../../../adapter/eslint'; import type { PrettierOutput } from '../../../adapter/prettier'; import type { StreamInterceptor } from '../../../lint/external'; +import type { InternalLintResult } from '../../internal'; import { createEslintAnnotations } from './eslint'; +import { createInternalAnnotations } from './internal'; import { createPrettierAnnotations } from './prettier'; import { createTscAnnotations } from './tsc'; export const createGitHubAnnotations = async ( + internal: InternalLintResult, eslint: ESLintOutput, prettier: PrettierOutput, tscOk: boolean, @@ -22,12 +25,13 @@ export const createGitHubAnnotations = async ( } const annotations: GitHub.Annotation[] = [ + ...createInternalAnnotations(internal), ...createEslintAnnotations(eslint), ...createPrettierAnnotations(prettier), ...createTscAnnotations(tscOk, tscOutputStream), ]; - const isOk = eslint.ok && prettier.ok && tscOk; + const isOk = eslint.ok && prettier.ok && internal.ok && tscOk; const summary = isOk ? '`skuba lint` passed.' diff --git a/src/cli/lint/annotate/github/internal.ts b/src/cli/lint/annotate/github/internal.ts new file mode 100644 index 000000000..9893fdea2 --- /dev/null +++ b/src/cli/lint/annotate/github/internal.ts @@ -0,0 +1,14 @@ +import type * as GitHub from '../../../../api/github'; +import type { InternalLintResult } from '../../internal'; + +export const createInternalAnnotations = ( + internal: InternalLintResult, +): GitHub.Annotation[] => + (internal.annotations ?? []).map((annotation) => ({ + annotation_level: 'failure', + start_line: annotation.start_line ?? 1, + end_line: annotation.end_line ?? annotation.start_line ?? 1, + path: annotation.path, + message: annotation.message, + title: 'skuba lint', + })); diff --git a/src/cli/lint/annotate/index.ts b/src/cli/lint/annotate/index.ts index b150d85b3..59a7eeb95 100644 --- a/src/cli/lint/annotate/index.ts +++ b/src/cli/lint/annotate/index.ts @@ -1,18 +1,26 @@ import type { ESLintOutput } from '../../../cli/adapter/eslint'; import type { PrettierOutput } from '../../../cli/adapter/prettier'; import type { StreamInterceptor } from '../external'; +import type { InternalLintResult } from '../internal'; import { createBuildkiteAnnotations } from './buildkite'; import { createGitHubAnnotations } from './github'; export const createAnnotations = async ( + internal: InternalLintResult, eslint: ESLintOutput, prettier: PrettierOutput, tscOk: boolean, tscOutputStream: StreamInterceptor, ): Promise => { await Promise.all([ - createGitHubAnnotations(eslint, prettier, tscOk, tscOutputStream), - createBuildkiteAnnotations(eslint, prettier, tscOk, tscOutputStream), + createGitHubAnnotations(internal, eslint, prettier, tscOk, tscOutputStream), + createBuildkiteAnnotations( + internal, + eslint, + prettier, + tscOk, + tscOutputStream, + ), ]); }; diff --git a/src/cli/lint/external.ts b/src/cli/lint/external.ts index 03354868f..6e4d6f74e 100644 --- a/src/cli/lint/external.ts +++ b/src/cli/lint/external.ts @@ -1,10 +1,5 @@ import stream from 'stream'; -import { inspect } from 'util'; -import { log } from '../../utils/logging'; -import { throwOnTimeout } from '../../utils/wait'; - -import { createAnnotations } from './annotate'; import { runESLintInCurrentThread, runESLintInWorkerThread } from './eslint'; import { runPrettierInCurrentThread, @@ -85,31 +80,10 @@ export const externalLint = async (input: Input) => { const { eslint, prettier, tscOk } = await lint({ ...input, tscOutputStream }); - try { - await throwOnTimeout( - createAnnotations(eslint, prettier, tscOk, tscOutputStream), - { s: 30 }, - ); - } catch (err) { - log.warn('Failed to annotate lint results.'); - log.subtle(inspect(err)); - } - - if (!eslint.ok || !prettier.ok || !tscOk) { - const tools = [ - ...(eslint.ok ? [] : ['ESLint']), - ...(prettier.ok ? [] : ['Prettier']), - ...(tscOk ? [] : ['tsc']), - ]; - - log.newline(); - log.err(`${tools.join(', ')} found issues that require triage.`); - - process.exitCode = 1; - } - return { eslint, prettier, + tscOk, + tscOutputStream, }; }; diff --git a/src/cli/lint/index.ts b/src/cli/lint/index.ts index 1db48060c..346b7b2c3 100644 --- a/src/cli/lint/index.ts +++ b/src/cli/lint/index.ts @@ -1,7 +1,12 @@ import type { Writable } from 'stream'; +import { inspect } from 'util'; import { hasDebugFlag, hasSerialFlag } from '../../utils/args'; +import { log } from '../../utils/logging'; +import { detectPackageManager } from '../../utils/packageManager'; +import { throwOnTimeout } from '../../utils/wait'; +import { createAnnotations } from './annotate'; import { autofix } from './autofix'; import { externalLint } from './external'; import { internalLint } from './internal'; @@ -9,23 +14,58 @@ import type { Input } from './types'; export const lint = async ( args = process.argv.slice(2), - tscOutputStream: Writable | undefined = undefined, + tscWriteable: Writable | undefined = undefined, workerThreads = true, ) => { const opts: Input = { debug: hasDebugFlag(args), serial: hasSerialFlag(args), - tscOutputStream, + tscOutputStream: tscWriteable, workerThreads, }; - const external = await externalLint(opts); + const { eslint, prettier, tscOk, tscOutputStream } = await externalLint(opts); const internal = await internalLint('lint', opts); + try { + await throwOnTimeout( + createAnnotations(internal, eslint, prettier, tscOk, tscOutputStream), + { s: 30 }, + ); + } catch (err) { + log.warn('Failed to annotate lint results.'); + log.subtle(inspect(err)); + } + + if (!internal.ok || !eslint.ok || !prettier.ok || !tscOk) { + process.exitCode = 1; + + const tools = [ + ...(internal.ok ? [] : ['skuba']), + ...(eslint.ok ? [] : ['ESLint']), + ...(prettier.ok ? [] : ['Prettier']), + ...(tscOk ? [] : ['tsc']), + ]; + + log.err(`${tools.join(', ')} found issues that require triage.`); + + if (internal.fixable || eslint.fixable || !prettier.ok) { + const packageManager = await detectPackageManager(); + log.newline(); + log.warn( + `Try running ${log.bold( + packageManager.exec, + 'skuba', + 'format', + )} to fix them.`, + ); + } + } + await autofix({ debug: opts.debug, - eslint: external.eslint.fixable, - prettier: !external.prettier.ok, + eslint: eslint.fixable, + prettier: !prettier.ok, internal: internal.fixable, }); }; diff --git a/src/cli/lint/internal.test.ts b/src/cli/lint/internal.test.ts index 59b986757..05d85b5d0 100644 --- a/src/cli/lint/internal.test.ts +++ b/src/cli/lint/internal.test.ts @@ -15,5 +15,6 @@ describe('internalLint', () => { expect(internalLint('lint')).resolves.toEqual({ ok: true, fixable: false, + annotations: [], })); }); diff --git a/src/cli/lint/internal.ts b/src/cli/lint/internal.ts index 5724d506e..f13bec3a8 100644 --- a/src/cli/lint/internal.ts +++ b/src/cli/lint/internal.ts @@ -3,7 +3,6 @@ import { inspect } from 'util'; import chalk from 'chalk'; import { type Logger, createLogger } from '../../utils/logging'; -import { detectPackageManager } from '../../utils/packageManager'; import { upgradeSkuba } from '../configure/upgrade'; import { deleteFilesLint } from './internalLints/deleteFiles'; @@ -11,15 +10,23 @@ import { noSkubaTemplateJs } from './internalLints/noSkubaTemplateJs'; import { tryRefreshIgnoreFiles } from './internalLints/refreshIgnoreFiles'; import type { Input } from './types'; -const lints = [ - deleteFilesLint, - noSkubaTemplateJs, - tryRefreshIgnoreFiles, - upgradeSkuba, -]; +export type InternalLintResult = { + ok: boolean; + fixable: boolean; + annotations?: Array<{ + start_line?: number; + end_line?: number; + path: string; + message: string; + }>; +}; + +const lints: Array< + (mode: 'format' | 'lint', logger: Logger) => Promise +> = [deleteFilesLint, noSkubaTemplateJs, tryRefreshIgnoreFiles, upgradeSkuba]; const lintSerially = async (mode: 'format' | 'lint', logger: Logger) => { - const results = []; + const results: InternalLintResult[] = []; for (const lint of lints) { results.push(await lint(mode, logger)); } @@ -34,7 +41,10 @@ const selectLintFunction = (input?: Input) => { return isSerial ? lintSerially : lintConcurrently; }; -export const internalLint = async (mode: 'format' | 'lint', input?: Input) => { +export const internalLint = async ( + mode: 'format' | 'lint', + input?: Input, +): Promise => { const start = process.hrtime.bigint(); const logger = createLogger( input?.debug ?? false, @@ -44,7 +54,7 @@ export const internalLint = async (mode: 'format' | 'lint', input?: Input) => { try { const lint = selectLintFunction(input); const results = await lint(mode, logger); - const result = await processResults(results, logger); + const result = combineResults(results); const end = process.hrtime.bigint(); logger.plain(`Processed skuba lints in ${logger.timing(start, end)}.`); return result; @@ -54,42 +64,16 @@ export const internalLint = async (mode: 'format' | 'lint', input?: Input) => { process.exitCode = 1; - return { ok: false, fixable: false }; + return { ok: false, fixable: false, annotations: [] }; } }; -const processResults = async ( - results: Array<{ ok: boolean; fixable: boolean }>, - logger: Logger, -) => { - const result = results.reduce( +const combineResults = (results: InternalLintResult[]): InternalLintResult => + results.reduce( (cur, next) => ({ ok: cur.ok && next.ok, fixable: cur.fixable || next.fixable, + annotations: [...(cur.annotations ?? []), ...(next.annotations ?? [])], }), { ok: true, fixable: false }, ); - - if (!result.ok) { - process.exitCode = 1; - - if (result.fixable) { - const packageManager = await detectPackageManager(); - logger.err( - `Some skuba lints failed. Try running ${logger.bold( - packageManager.exec, - 'skuba', - 'format', - )} to fix them. ${logger.dim('skuba-lint')}`, - ); - } else { - logger.err( - `Some skuba lints failed which require triage. ${logger.dim( - 'skuba-lint', - )}`, - ); - } - } - - return result; -}; diff --git a/src/cli/lint/internalLints/deleteFiles.test.ts b/src/cli/lint/internalLints/deleteFiles.test.ts index dc8a2052a..533199fd1 100644 --- a/src/cli/lint/internalLints/deleteFiles.test.ts +++ b/src/cli/lint/internalLints/deleteFiles.test.ts @@ -50,6 +50,12 @@ describe('deleteFilesLint', () => { await expect(deleteFilesLint('lint', log)).resolves.toEqual({ ok: false, fixable: true, + annotations: [ + { + message: 'This file should be deleted.', + path: 'Dockerfile-incunabulum', + }, + ], }); expect(stdout()).toBe( diff --git a/src/cli/lint/internalLints/deleteFiles.ts b/src/cli/lint/internalLints/deleteFiles.ts index e59513231..c46011ffe 100644 --- a/src/cli/lint/internalLints/deleteFiles.ts +++ b/src/cli/lint/internalLints/deleteFiles.ts @@ -5,6 +5,7 @@ import { pathExists, rm } from 'fs-extra'; import type { Logger } from '../../../utils/logging'; import { detectPackageManager } from '../../../utils/packageManager'; +import type { InternalLintResult } from '../internal'; const AUTOFIX_DELETE_FILES = [ // Try to delete this SEEK-Jobs/gutenberg automation file that may have been @@ -15,7 +16,7 @@ const AUTOFIX_DELETE_FILES = [ export const deleteFilesLint = async ( mode: 'format' | 'lint', logger: Logger, -) => { +): Promise => { const dir = process.cwd(); const toDelete = ( @@ -71,6 +72,10 @@ export const deleteFilesLint = async ( return { ok: false, fixable: true, + annotations: toDelete.map((filename) => ({ + path: filename, + message: 'This file should be deleted.', + })), }; } diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts index 7ed4e281d..a35c6466b 100644 --- a/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts @@ -49,6 +49,12 @@ describe('noSkubaTemplateJs', () => { await expect(noSkubaTemplateJs(mode, log)).resolves.toEqual({ ok: false, fixable: false, + annotations: [ + { + message: 'Template is incomplete; run pnpm exec skuba configure.', + path: '/Users/amoat/Programming/seek-oss/skuba/skuba.template.js', + }, + ], }); expect(stdout()).toBe( diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.ts index bfe48a05a..853ddd347 100644 --- a/src/cli/lint/internalLints/noSkubaTemplateJs.ts +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.ts @@ -5,11 +5,12 @@ import { pathExists } from 'fs-extra'; import type { Logger } from '../../../utils/logging'; import { getConsumerManifest } from '../../../utils/manifest'; import { detectPackageManager } from '../../../utils/packageManager'; +import type { InternalLintResult } from '../internal'; export const noSkubaTemplateJs = async ( _mode: 'format' | 'lint', logger: Logger, -) => { +): Promise => { const [manifest, packageManager] = await Promise.all([ getConsumerManifest(), detectPackageManager(), @@ -34,7 +35,16 @@ export const noSkubaTemplateJs = async ( )}. ${logger.dim('no-skuba-template-js')}`, ); - return { ok: false, fixable: false }; + return { + ok: false, + fixable: false, + annotations: [ + { + path: templateConfigPath, + message: `Template is incomplete; run ${packageManager.exec} skuba configure.`, + }, + ], + }; } return { ok: true, fixable: false }; diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts index 5f1789859..c9937a5a8 100644 --- a/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts @@ -58,6 +58,7 @@ describe('refreshIgnoreFiles', () => { await expect(refreshIgnoreFiles('lint', log)).resolves.toEqual({ ok: true, fixable: false, + annotations: [], }); expect(stdout()).toBe(''); @@ -78,6 +79,18 @@ describe('refreshIgnoreFiles', () => { await expect(refreshIgnoreFiles('lint', log)).resolves.toEqual({ ok: false, fixable: true, + annotations: [ + { + message: + 'The .eslintignore file is out of date. Run `pnpm exec skuba format` to update it.', + path: '.eslintignore', + }, + { + message: + 'The .gitignore file is out of date. Run `pnpm exec skuba format` to update it.', + path: '.gitignore', + }, + ], }); expect(`\n${stdout()}`).toBe(` @@ -103,6 +116,7 @@ The .gitignore file is out of date. Run pnpm exec skuba format to update it. ref await expect(refreshIgnoreFiles('format', log)).resolves.toEqual({ ok: true, fixable: false, + annotations: [], }); expect(stdout()).toBe(''); @@ -123,6 +137,7 @@ The .gitignore file is out of date. Run pnpm exec skuba format to update it. ref await expect(refreshIgnoreFiles('format', log)).resolves.toEqual({ ok: true, fixable: false, + annotations: [], }); expect(stdout()).toBe( diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.ts index 64890da3a..1171d54a8 100644 --- a/src/cli/lint/internalLints/refreshIgnoreFiles.ts +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.ts @@ -9,6 +9,7 @@ import { readBaseTemplateFile } from '../../../utils/template'; import { getDestinationManifest } from '../../configure/analysis/package'; import { createDestinationFileReader } from '../../configure/analysis/project'; import { mergeWithIgnoreFile } from '../../configure/processing/ignoreFile'; +import type { InternalLintResult } from '../internal'; const REFRESHABLE_IGNORE_FILES = [ '.eslintignore', @@ -19,7 +20,7 @@ const REFRESHABLE_IGNORE_FILES = [ export const refreshIgnoreFiles = async ( mode: 'format' | 'lint', logger: Logger, -) => { +): Promise => { // TODO: check current state of .gitignore // If it contains !.npmrc, break // If it contains .npmrc, we can either @@ -70,6 +71,8 @@ export const refreshIgnoreFiles = async ( 'skuba', 'format', )} to update it. ${logger.dim('refresh-ignore-files')}`, + filename, + annotationMessage: `The ${filename} file is out of date. Run \`${packageManager.exec} skuba format\` to update it.`, }; } @@ -92,13 +95,24 @@ export const refreshIgnoreFiles = async ( return { ok: !anyNeedChanging, fixable: anyNeedChanging, + annotations: results.flatMap( + ({ needsChange, filename, annotationMessage }) => + needsChange && annotationMessage + ? [ + { + path: filename, + message: annotationMessage, + }, + ] + : [], + ), }; }; export const tryRefreshIgnoreFiles = async ( mode: 'format' | 'lint', logger: Logger, -) => { +): Promise => { try { return await refreshIgnoreFiles(mode, logger); } catch (err) { @@ -108,6 +122,7 @@ export const tryRefreshIgnoreFiles = async ( return { ok: false, fixable: false, + annotations: [], }; } }; From 6ca5431d6358a783d19544edccef2e7bb587b453 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Sun, 7 Jan 2024 22:30:31 +1100 Subject: [PATCH 05/13] Fix test --- src/cli/lint/annotate/github/index.test.ts | 18 ++++++++++++++++++ .../internalLints/noSkubaTemplateJs.test.ts | 2 +- .../lint/internalLints/noSkubaTemplateJs.ts | 2 +- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/src/cli/lint/annotate/github/index.test.ts b/src/cli/lint/annotate/github/index.test.ts index a526d860a..0e5e7c77e 100644 --- a/src/cli/lint/annotate/github/index.test.ts +++ b/src/cli/lint/annotate/github/index.test.ts @@ -56,6 +56,12 @@ const prettierOutput: PrettierOutput = { const internalOutput: InternalLintResult = { ok: false, fixable: false, + annotations: [ + { + path: 'src/index.ts', + message: 'something is wrong about this', + }, + ], }; const tscOk = false; @@ -65,6 +71,17 @@ const tscOutputStream = { output: mockOutput, } as unknown as StreamInterceptor; +const mockInternalAnnotations: GitHub.Annotation[] = [ + { + annotation_level: 'failure', + end_line: 1, + message: 'something is wrong about this', + path: 'src/index.ts', + start_line: 1, + title: 'skuba lint', + }, +]; + const mockEslintAnnotations: GitHub.Annotation[] = [ { annotation_level: 'failure', @@ -178,6 +195,7 @@ it('should call createTscAnnotations with tscOk and tscOutputStream', async () = it('should combine all the annotations into an array for the check run', async () => { const expectedAnnotations: GitHub.Annotation[] = [ + ...mockInternalAnnotations, ...mockEslintAnnotations, ...mockPrettierAnnotations, ...mockTscAnnotations, diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts index a35c6466b..c02c402f6 100644 --- a/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.test.ts @@ -52,7 +52,7 @@ describe('noSkubaTemplateJs', () => { annotations: [ { message: 'Template is incomplete; run pnpm exec skuba configure.', - path: '/Users/amoat/Programming/seek-oss/skuba/skuba.template.js', + path: 'skuba.template.js', }, ], }); diff --git a/src/cli/lint/internalLints/noSkubaTemplateJs.ts b/src/cli/lint/internalLints/noSkubaTemplateJs.ts index 853ddd347..f164315da 100644 --- a/src/cli/lint/internalLints/noSkubaTemplateJs.ts +++ b/src/cli/lint/internalLints/noSkubaTemplateJs.ts @@ -40,7 +40,7 @@ export const noSkubaTemplateJs = async ( fixable: false, annotations: [ { - path: templateConfigPath, + path: 'skuba.template.js', message: `Template is incomplete; run ${packageManager.exec} skuba configure.`, }, ], From 1ad5f4442ee71fcf5ef5b73b7a2214ed5592efb2 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Mon, 8 Jan 2024 08:59:59 +1100 Subject: [PATCH 06/13] Don't fail skuba lint if all patches to apply are no-ops --- .eslintignore | 1 + integration/base/patch/Dockerfile | 1 + integration/base/patch/a/a/a.ts | 7 + integration/base/patch/b.md | 3 + integration/base/patch/c.json | 3 + integration/base/patch/d.js | 2 + integration/base/patch/package.json | 13 + integration/base/patch/tsconfig.json | 9 + src/cli/__snapshots__/format.int.test.ts.snap | 32 +- src/cli/configure/patchRenovateConfig.test.ts | 518 +++++++++++++----- src/cli/configure/patchRenovateConfig.ts | 43 +- src/cli/configure/upgrade/index.test.ts | 21 +- src/cli/configure/upgrade/index.ts | 64 ++- .../patches/7.3.1/addEmptyExports.test.ts | 166 ++++-- .../upgrade/patches/7.3.1/addEmptyExports.ts | 21 +- .../configure/upgrade/patches/7.3.1/index.ts | 28 +- .../patches/7.3.1/patchDockerfile.test.ts | 64 ++- .../upgrade/patches/7.3.1/patchDockerfile.ts | 26 +- .../patches/7.3.1/patchServerListener.test.ts | 361 ++++++++---- .../patches/7.3.1/patchServerListener.ts | 29 +- src/cli/init/index.ts | 2 +- src/cli/lint.int.test.ts | 2 +- 22 files changed, 1052 insertions(+), 364 deletions(-) create mode 100644 integration/base/patch/Dockerfile create mode 100644 integration/base/patch/a/a/a.ts create mode 100644 integration/base/patch/b.md create mode 100644 integration/base/patch/c.json create mode 100644 integration/base/patch/d.js create mode 100644 integration/base/patch/package.json create mode 100644 integration/base/patch/tsconfig.json diff --git a/.eslintignore b/.eslintignore index 8aeae2d4a..1ad3bee7d 100644 --- a/.eslintignore +++ b/.eslintignore @@ -15,4 +15,5 @@ node_modules*/ # end managed by skuba /integration/base/ +/integration/format/ /template/ diff --git a/integration/base/patch/Dockerfile b/integration/base/patch/Dockerfile new file mode 100644 index 000000000..c595da18b --- /dev/null +++ b/integration/base/patch/Dockerfile @@ -0,0 +1 @@ +FROM gcr.io/distroless/nodejs:18 AS runtime \ No newline at end of file diff --git a/integration/base/patch/a/a/a.ts b/integration/base/patch/a/a/a.ts new file mode 100644 index 000000000..6464ec106 --- /dev/null +++ b/integration/base/patch/a/a/a.ts @@ -0,0 +1,7 @@ +// Imports in order +import fs from 'fs'; +import path from 'path'; + +export const main = async () => { + await fs.promises.access(path.join('.', 'a.ts')); +}; diff --git a/integration/base/patch/b.md b/integration/base/patch/b.md new file mode 100644 index 000000000..9c28c8363 --- /dev/null +++ b/integration/base/patch/b.md @@ -0,0 +1,3 @@ +# Title + +No trailing space diff --git a/integration/base/patch/c.json b/integration/base/patch/c.json new file mode 100644 index 000000000..7a9e86441 --- /dev/null +++ b/integration/base/patch/c.json @@ -0,0 +1,3 @@ +{ + "key": "value" +} diff --git a/integration/base/patch/d.js b/integration/base/patch/d.js new file mode 100644 index 000000000..7765f523f --- /dev/null +++ b/integration/base/patch/d.js @@ -0,0 +1,2 @@ +// eslint-disable-next-line no-console +console.log(process.argv); diff --git a/integration/base/patch/package.json b/integration/base/patch/package.json new file mode 100644 index 000000000..1fcbe9922 --- /dev/null +++ b/integration/base/patch/package.json @@ -0,0 +1,13 @@ +{ + "private": true, + "license": "UNLICENSED", + "sideEffects": false, + "dependencies": {}, + "devDependencies": {}, + "skuba": { + "entryPoint": null, + "template": null, + "type": "application", + "version": "0.0.1" + } +} diff --git a/integration/base/patch/tsconfig.json b/integration/base/patch/tsconfig.json new file mode 100644 index 000000000..abaf52a88 --- /dev/null +++ b/integration/base/patch/tsconfig.json @@ -0,0 +1,9 @@ +{ + "compilerOptions": { + "incremental": true, + "moduleResolution": "node", + "outDir": "lib", + "skipLibCheck": true + }, + "extends": "tsconfig-seek" +} diff --git a/src/cli/__snapshots__/format.int.test.ts.snap b/src/cli/__snapshots__/format.int.test.ts.snap index 4fd545bb1..38fa0e33f 100644 --- a/src/cli/__snapshots__/format.int.test.ts.snap +++ b/src/cli/__snapshots__/format.int.test.ts.snap @@ -8,7 +8,13 @@ Refreshed .gitignore. refresh-ignore-files Refreshed .prettierignore. refresh-ignore-files Updating skuba... -Patch 7.3.1 applied. +Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules + +Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config + +Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found + +Patch skipped: Add keepAliveTimeout to server listener - no listener file found skuba update complete. @@ -69,7 +75,13 @@ Refreshed .gitignore. refresh-ignore-files Refreshed .prettierignore. refresh-ignore-files Updating skuba... -Patch 7.3.1 applied. +Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules + +Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config + +Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found + +Patch skipped: Add keepAliveTimeout to server listener - no listener file found skuba update complete. @@ -123,7 +135,13 @@ Refreshed .gitignore. refresh-ignore-files Refreshed .prettierignore. refresh-ignore-files Updating skuba... -Patch 7.3.1 applied. +Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules + +Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config + +Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found + +Patch skipped: Add keepAliveTimeout to server listener - no listener file found skuba update complete. @@ -150,7 +168,13 @@ Refreshed .gitignore. refresh-ignore-files Refreshed .prettierignore. refresh-ignore-files Updating skuba... -Patch 7.3.1 applied. +Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules + +Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config + +Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found + +Patch skipped: Add keepAliveTimeout to server listener - no listener file found skuba update complete. diff --git a/src/cli/configure/patchRenovateConfig.test.ts b/src/cli/configure/patchRenovateConfig.test.ts index c707d608a..4c554a2f6 100644 --- a/src/cli/configure/patchRenovateConfig.test.ts +++ b/src/cli/configure/patchRenovateConfig.test.ts @@ -41,191 +41,443 @@ const volToJson = () => vol.toJSON(process.cwd(), undefined, true); beforeEach(jest.clearAllMocks); beforeEach(() => vol.reset()); -it('patches a JSON config for a SEEK-Jobs project', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'VersionNet' }); - - vol.fromJSON({ '.git': null, 'renovate.json': JSON }); - - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); - - expect(volToJson()).toMatchInlineSnapshot(` - { - ".git": null, - "renovate.json": "{ - "extends": [ - "local>seek-jobs/renovate-config", - "github>seek-oss/rynovate:third-party-major" - ] - } - ", - } - `); -}); +describe('patchRenovateConfig', () => { + describe('format mode', () => { + it('patches a JSON config for a SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'VersionNet', + }); + + vol.fromJSON({ '.git': null, 'renovate.json': JSON }); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toMatchInlineSnapshot(` + { + ".git": null, + "renovate.json": "{ + "extends": [ + "local>seek-jobs/renovate-config", + "github>seek-oss/rynovate:third-party-major" + ] + } + ", + } + `); + }); + + it('patches a JSON config for a new SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'VersionNet', + }); + + vol.fromJSON({ 'foo/.git': null, 'foo/renovate.json': JSON }); + + await expect(tryPatchRenovateConfig('format', 'foo')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toMatchInlineSnapshot(` + { + "foo/.git": null, + "foo/renovate.json": "{ + "extends": [ + "local>seek-jobs/renovate-config", + "github>seek-oss/rynovate:third-party-major" + ] + } + ", + } + `); + }); + + it('patches a JSON5 config for a seekasia project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'sEEkAsIa', + repo: 'VersionCobol', + }); + + vol.fromJSON({ '.git': null, '.github/renovate.json5': JSON5 }); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'apply', + }); + + // Note that `golden-fleece` can't do any better than this imperfect output, + // but at least it allows us to preserve the comments rather than dropping + // them entirely. + expect(volToJson()).toMatchInlineSnapshot(` + { + ".git": null, + ".github/renovate.json5": "{ + extends: [ + // Preceding comment + 'local>seekasia/renovate-config', + + 'seek', + // Succeeding comment + ], + } + ", + } + `); + }); + + it('handles a lack of Renovate config', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); + + const files = { '.git': null }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: 'no renovate config found', + }); + + expect(volToJson()).toStrictEqual(files); + }); -it('patches a JSON config for a new SEEK-Jobs project', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'VersionNet' }); - - vol.fromJSON({ 'foo/.git': null, 'foo/renovate.json': JSON }); - - await expect(tryPatchRenovateConfig('foo')).resolves.toBeUndefined(); - - expect(volToJson()).toMatchInlineSnapshot(` - { - "foo/.git": null, - "foo/renovate.json": "{ - "extends": [ - "local>seek-jobs/renovate-config", - "github>seek-oss/rynovate:third-party-major" - ] - } - ", - } - `); -}); + it('handles a filesystem error', async () => { + const err = new Error('Badness!'); + + writeFile.mockRejectedValueOnce(err); + + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'VersionNet', + }); + + const files = { '.git': null, 'renovate.json5': JSON5 }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: 'due to an error', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(consoleLog).toHaveBeenCalledWith( + 'Failed to patch Renovate config.', + ); + expect(consoleLog).toHaveBeenCalledWith(inspect(err)); + }); + + it('handles a non-Git directory', async () => { + const err = new Error('Badness!'); + + getOwnerAndRepo.mockRejectedValue(err); + + const files = {}; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: 'could not find git root', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(consoleLog).not.toHaveBeenCalled(); + }); + + it('skips a seek-oss project', async () => { + getOwnerAndRepo.mockResolvedValue({ owner: 'seek-oss', repo: 'skuba' }); + + const files = { '.git': null, 'renovate.json5': JSON5 }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: "owner does not map to any of SEEK's renovate auth config", + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a personal project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'Seekie1337', + repo: 'fizz-buzz', + }); + + const files = { '.git': null, '.renovaterc': JSON }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: "owner does not map to any of SEEK's renovate auth config", + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a strange config without `extends`', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); -it('patches a JSON5 config for a seekasia project', async () => { - getOwnerAndRepo.mockResolvedValue({ - owner: 'sEEkAsIa', - repo: 'VersionCobol', + const files = { '.git': null, '.github/renovate.json5': '{}' }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a configured SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); + + const files = { + '.git': null, + '.github/renovate.json5': JSON5_CONFIGURED, + }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: 'renovate config already has private auth', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a SEEK-Jobs project which already extends a config', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); + + const files = { '.git': null, '.github/renovate.json5': JSON5_EXTENDED }; + + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ + result: 'skip', + reason: 'renovate config already has private auth', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); }); - vol.fromJSON({ '.git': null, '.github/renovate.json5': JSON5 }); - - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); - - // Note that `golden-fleece` can't do any better than this imperfect output, - // but at least it allows us to preserve the comments rather than dropping - // them entirely. - expect(volToJson()).toMatchInlineSnapshot(` - { - ".git": null, - ".github/renovate.json5": "{ - extends: [ - // Preceding comment - 'local>seekasia/renovate-config', - - 'seek', - // Succeeding comment - ], - } - ", - } - `); -}); + describe('lint mode', () => { + it('patches a JSON config for a SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'VersionNet', + }); -it('handles a lack of Renovate config', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'monolith' }); + vol.fromJSON({ '.git': null, 'renovate.json': JSON }); - const files = { '.git': null }; + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'apply', + }); - vol.fromJSON(files); + expect(volToJson()).toEqual({ '.git': null, 'renovate.json': JSON }); + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + it('patches a JSON config for a new SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'VersionNet', + }); - expect(volToJson()).toStrictEqual(files); -}); + vol.fromJSON({ 'foo/.git': null, 'foo/renovate.json': JSON }); -it('handles a filesystem error', async () => { - const err = new Error('Badness!'); + await expect(tryPatchRenovateConfig('lint', 'foo')).resolves.toEqual({ + result: 'apply', + }); - writeFile.mockRejectedValueOnce(err); + expect(volToJson()).toEqual({ + 'foo/.git': null, + 'foo/renovate.json': JSON, + }); + }); - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'VersionNet' }); + it('patches a JSON5 config for a seekasia project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'sEEkAsIa', + repo: 'VersionCobol', + }); - const files = { '.git': null, 'renovate.json5': JSON5 }; + vol.fromJSON({ '.git': null, '.github/renovate.json5': JSON5 }); - vol.fromJSON(files); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'apply', + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + // unchanged + expect(volToJson()).toEqual({ + '.git': null, + '.github/renovate.json5': JSON5, + }); + }); - expect(volToJson()).toStrictEqual(files); + it('handles a lack of Renovate config', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); - expect(consoleLog).toHaveBeenCalledWith('Failed to patch Renovate config.'); - expect(consoleLog).toHaveBeenCalledWith(inspect(err)); -}); + const files = { '.git': null }; -it('handles a non-Git directory', async () => { - const err = new Error('Badness!'); + vol.fromJSON(files); - getOwnerAndRepo.mockRejectedValue(err); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: 'no renovate config found', + }); - const files = {}; + expect(volToJson()).toStrictEqual(files); + }); - vol.fromJSON(files); + it('handles a non-Git directory', async () => { + const err = new Error('Badness!'); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + getOwnerAndRepo.mockRejectedValue(err); - expect(volToJson()).toStrictEqual(files); + const files = {}; - expect(consoleLog).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); -it('skips a seek-oss project', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'seek-oss', repo: 'skuba' }); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: 'could not find git root', + }); - const files = { '.git': null, 'renovate.json5': JSON5 }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(consoleLog).not.toHaveBeenCalled(); + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + it('skips a seek-oss project', async () => { + getOwnerAndRepo.mockResolvedValue({ owner: 'seek-oss', repo: 'skuba' }); - expect(volToJson()).toStrictEqual(files); + const files = { '.git': null, 'renovate.json5': JSON5 }; - expect(writeFile).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); -it('skips a personal project', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'Seekie1337', repo: 'fizz-buzz' }); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: "owner does not map to any of SEEK's renovate auth config", + }); - const files = { '.git': null, '.renovaterc': JSON }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + it('skips a personal project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'Seekie1337', + repo: 'fizz-buzz', + }); - expect(volToJson()).toStrictEqual(files); + const files = { '.git': null, '.renovaterc': JSON }; - expect(writeFile).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); -it('skips a strange config without `extends`', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'monolith' }); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: "owner does not map to any of SEEK's renovate auth config", + }); - const files = { '.git': null, '.github/renovate.json5': '{}' }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + it('skips a strange config without `extends`', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); - expect(volToJson()).toStrictEqual(files); + const files = { '.git': null, '.github/renovate.json5': '{}' }; - expect(writeFile).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); -it('skips a configured SEEK-Jobs project', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'monolith' }); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'apply', + }); - const files = { '.git': null, '.github/renovate.json5': JSON5_CONFIGURED }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + it('skips a configured SEEK-Jobs project', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); - expect(volToJson()).toStrictEqual(files); + const files = { + '.git': null, + '.github/renovate.json5': JSON5_CONFIGURED, + }; - expect(writeFile).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); + + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: 'renovate config already has private auth', + }); -it('skips a SEEK-Jobs project which already extends a config', async () => { - getOwnerAndRepo.mockResolvedValue({ owner: 'SEEK-Jobs', repo: 'monolith' }); + expect(volToJson()).toStrictEqual(files); - const files = { '.git': null, '.github/renovate.json5': JSON5_EXTENDED }; + expect(writeFile).not.toHaveBeenCalled(); + }); - vol.fromJSON(files); + it('skips a SEEK-Jobs project which already extends a config', async () => { + getOwnerAndRepo.mockResolvedValue({ + owner: 'SEEK-Jobs', + repo: 'monolith', + }); - await expect(tryPatchRenovateConfig()).resolves.toBeUndefined(); + const files = { '.git': null, '.github/renovate.json5': JSON5_EXTENDED }; - expect(volToJson()).toStrictEqual(files); + vol.fromJSON(files); - expect(writeFile).not.toHaveBeenCalled(); + await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ + result: 'skip', + reason: 'renovate config already has private auth', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/cli/configure/patchRenovateConfig.ts b/src/cli/configure/patchRenovateConfig.ts index ec70508c8..6d8a3645f 100644 --- a/src/cli/configure/patchRenovateConfig.ts +++ b/src/cli/configure/patchRenovateConfig.ts @@ -11,6 +11,7 @@ import { log } from '../../utils/logging'; import { createDestinationFileReader } from './analysis/project'; import { RENOVATE_CONFIG_FILENAMES } from './modules/renovate'; import { formatPrettier } from './processing/prettier'; +import type { PatchFunction, PatchReturnType } from './upgrade'; const RENOVATE_PRESETS = [ 'local>seekasia/renovate-config', @@ -91,7 +92,10 @@ const patchByFiletype: Record = { json5: patchJson5, }; -const patchRenovateConfig = async (dir: string) => { +const patchRenovateConfig = async ( + mode: 'format' | 'lint', + dir: string, +): Promise => { const readFile = createDestinationFileReader(dir); const { owner } = await Git.getOwnerAndRepo({ dir }); @@ -99,8 +103,10 @@ const patchRenovateConfig = async (dir: string) => { const presetToAdd = ownerToRenovatePreset(owner); if (!presetToAdd) { - // No baseline preset needs to be added for the configured Git owner. - return; + return { + result: 'skip', + reason: "owner does not map to any of SEEK's renovate auth config", + }; } const maybeConfigs = await Promise.all( @@ -111,10 +117,11 @@ const patchRenovateConfig = async (dir: string) => { ); const config = maybeConfigs.find((maybeConfig) => Boolean(maybeConfig.input)); + if (!config?.input) { + return { result: 'skip', reason: 'no renovate config found' }; + } if ( - // No file was found. - !config?.input || // The file appears to mention the baseline preset for the configured Git // owner. This is a very naive check that we don't want to overcomplicate // because it is invoked before each skuba format and lint. @@ -122,7 +129,14 @@ const patchRenovateConfig = async (dir: string) => { // Ignore any renovate configuration which already extends a SEEK-Jobs or seekasia config EXISTING_REPO_PRESET_REGEX.exec(config.input) ) { - return; + return { + result: 'skip', + reason: 'renovate config already has private auth', + }; + } + + if (mode === 'lint') { + return { result: 'apply' }; } const filetype: RenovateFiletype = config.filepath @@ -138,19 +152,26 @@ const patchRenovateConfig = async (dir: string) => { input: config.input, presetToAdd, }); + + return { result: 'apply' }; }; -export const tryPatchRenovateConfig = async (dir = process.cwd()) => { +export const tryPatchRenovateConfig = (async ( + mode: 'format' | 'lint', + dir = process.cwd(), +) => { try { // In a monorepo we may be invoked within a subdirectory, but we are working // with Renovate config that should be relative to the repository root. const gitRoot = await Git.findRoot({ dir }); - - if (gitRoot) { - await patchRenovateConfig(gitRoot); + if (!gitRoot) { + return { result: 'skip', reason: 'could not find git root' }; } + + return await patchRenovateConfig(mode, gitRoot); } catch (err) { log.warn('Failed to patch Renovate config.'); log.subtle(inspect(err)); + return { result: 'skip', reason: 'due to an error' }; } -}; +}) satisfies PatchFunction; diff --git a/src/cli/configure/upgrade/index.test.ts b/src/cli/configure/upgrade/index.test.ts index aae125057..f4be7c3fc 100644 --- a/src/cli/configure/upgrade/index.test.ts +++ b/src/cli/configure/upgrade/index.test.ts @@ -51,16 +51,17 @@ describe('upgradeSkuba in format mode', () => { it('should apply patches which are equal to or greater than the manifest version', async () => { const mockUpgrade = { - upgrade: jest.fn(), + apply: jest.fn().mockImplementation(() => ({ result: 'apply' })), + description: 'mock', }; - jest.mock(`./patches/0.9.0/index.js`, () => mockUpgrade, { + jest.mock(`./patches/0.9.0/index.js`, () => ({ patches: [mockUpgrade] }), { virtual: true, }); - jest.mock(`./patches/1.0.0/index.js`, () => mockUpgrade, { + jest.mock(`./patches/1.0.0/index.js`, () => ({ patches: [mockUpgrade] }), { virtual: true, }); - jest.mock(`./patches/2.0.0/index.js`, () => mockUpgrade, { + jest.mock(`./patches/2.0.0/index.js`, () => ({ patches: [mockUpgrade] }), { virtual: true, }); @@ -88,15 +89,16 @@ describe('upgradeSkuba in format mode', () => { ok: true, fixable: false, }); - expect(mockUpgrade.upgrade).toHaveBeenCalledTimes(2); + expect(mockUpgrade.apply).toHaveBeenCalledTimes(2); }); it('should update the consumer manifest version', async () => { const mockUpgrade = { - upgrade: jest.fn(), + apply: jest.fn().mockImplementation(() => ({ result: 'apply' })), + description: 'mock', }; - jest.mock(`./patches/2.0.0/index.js`, () => mockUpgrade, { + jest.mock(`./patches/2.0.0/index.js`, () => ({ patches: [mockUpgrade] }), { virtual: true, }); @@ -138,10 +140,11 @@ describe('upgradeSkuba in format mode', () => { it('should handle skuba section not being present in the packageJson', async () => { const mockUpgrade = { - upgrade: jest.fn(), + apply: jest.fn().mockImplementation(() => ({ result: 'apply' })), + description: 'mock', }; - jest.mock(`./patches/2.0.0/index.js`, () => mockUpgrade, { + jest.mock(`./patches/2.0.0/index.js`, () => ({ patches: [mockUpgrade] }), { virtual: true, }); diff --git a/src/cli/configure/upgrade/index.ts b/src/cli/configure/upgrade/index.ts index a7aab337d..babdddd46 100644 --- a/src/cli/configure/upgrade/index.ts +++ b/src/cli/configure/upgrade/index.ts @@ -11,32 +11,44 @@ import type { SkubaPackageJson } from '../../init/writePackageJson'; import type { InternalLintResult } from '../../lint/internal'; import { formatPackage } from '../processing/package'; -const getPatches = async (manifestVersion: string): Promise => { +export type Patches = Patch[]; +export type Patch = { + apply: PatchFunction; + description: string; +}; +export type PatchReturnType = + | { result: 'apply' } + | { result: 'skip'; reason?: string }; +export type PatchFunction = ( + mode: 'format' | 'lint', +) => Promise; + +const getPatches = async (manifestVersion: string): Promise => { const patches = await readdir(path.join(__dirname, 'patches')); // The patches are sorted by the version they were added from. // Only return patches that are newer or equal to the current version. - return sort(patches.filter((filename) => gte(filename, manifestVersion))); + const patchesForVersion = sort( + patches.filter((filename) => gte(filename, manifestVersion)), + ); + + return (await Promise.all(patchesForVersion.map(resolvePatches))).flat(); }; const fileExtensions = ['js', 'ts']; -export type Patch = { - upgrade: () => Promise; -}; - // Hack to allow our Jest environment/transform to resolve the patches // In normal scenarios this will resolve immediately after the .js import -const resolvePatch = async (patch: string): Promise => { +const resolvePatches = async (version: string): Promise => { for (const extension of fileExtensions) { try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return await import(`./patches/${patch}/index.${extension}`); + // eslint-disable-next-line @typescript-eslint/no-unsafe-return, @typescript-eslint/no-unsafe-member-access + return (await import(`./patches/${version}/index.${extension}`)).patches; } catch { // Ignore } } - throw new Error(`Could not resolve patch ${patch}`); + throw new Error(`Could not resolve patches for ${version}`); }; export const upgradeSkuba = async ( @@ -63,17 +75,20 @@ export const upgradeSkuba = async ( } const patches = await getPatches(manifestVersion); - + // No patches to apply even if version out of date. Early exit to avoid unnecessary commits. if (patches.length === 0) { - // TODO: Do we want to _always_ write the version? - // Or only if there's associated patches for that version / it's missing? - // Also see 'should return ok: true, fixable: false if there are no lints to apply despite package.json being out of date' return { ok: true, fixable: false }; } if (mode === 'lint') { - // TODO: Do we want to declare patches as optional? - // TODO: Should we return ok: true if all patches are no-ops? + const results = await Promise.all( + patches.map(async ({ apply }) => await apply(mode)), + ); + + // No patches are applicable. Early exit to avoid unnecessary commits. + if (results.every(({ result }) => result === 'skip')) { + return { ok: true, fixable: false }; + } const packageManager = await detectPackageManager(); @@ -91,7 +106,7 @@ export const upgradeSkuba = async ( annotations: [ { // package.json as likely skuba version has changed - // TODO: location the "skuba": {} config in the package.json and annotate on the version property + // TODO: locate the "skuba": {} config in the package.json and annotate on the version property path: manifest.path, message: `skuba has patches to apply. Run ${packageManager.exec} skuba format to run them.`, }, @@ -102,11 +117,18 @@ export const upgradeSkuba = async ( logger.plain('Updating skuba...'); // Run these in series in case a subsequent patch relies on a previous patch - for (const patch of patches) { - const patchFile = await resolvePatch(patch); - await patchFile.upgrade(); + for (const { apply, description } of patches) { + const result = await apply(mode); logger.newline(); - logger.plain(`Patch ${patch} applied.`); + if (result.result === 'skip') { + logger.plain( + `Patch skipped: ${description}${ + result.reason ? ` - ${result.reason}` : '' + }`, + ); + } else { + logger.plain(`Patch applied: ${description}`); + } } (manifest.packageJson.skuba as SkubaPackageJson).version = currentVersion; diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts index 6c4ee900a..3c8c49a72 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.test.ts @@ -20,14 +20,17 @@ const writeFile = jest.spyOn(fs.promises, 'writeFile').mockResolvedValue(); beforeEach(jest.clearAllMocks); describe('tryAddEmptyExports', () => { - it('converts non-compliant Jest setup files', async () => { - createDestinationFileReader.mockReturnValue((filename) => - Promise.resolve(`// ${filename}`), - ); + describe('format mode', () => { + it('converts non-compliant Jest setup files', async () => { + createDestinationFileReader.mockReturnValue((filename) => + Promise.resolve(`// ${filename}`), + ); - await expect(tryAddEmptyExports()).resolves.toBeUndefined(); + await expect(tryAddEmptyExports('format')).resolves.toEqual({ + result: 'apply', + }); - expect(writeFile.mock.calls.flat().join('\n')).toMatchInlineSnapshot(` + expect(writeFile.mock.calls.flat().join('\n')).toMatchInlineSnapshot(` "~/project/jest.setup.ts // jest.setup.ts @@ -39,53 +42,130 @@ describe('tryAddEmptyExports', () => { export {}; " `); - }); + }); - it('no-ops compliant Jest setup files', async () => { - createDestinationFileReader.mockReturnValue( - (filename) => - new Promise((resolve, reject) => { - switch (filename) { - case 'jest.setup.ts': - return resolve("import './register';"); - case 'jest.setup.int.ts': - return resolve('export const foo = true;'); - default: - return reject(new Error(`Not implemented: ${filename}`)); - } - }), - ); - - await expect(tryAddEmptyExports()).resolves.toBeUndefined(); - - expect(writeFile).not.toHaveBeenCalled(); - }); + it('no-ops compliant Jest setup files', async () => { + createDestinationFileReader.mockReturnValue( + (filename) => + new Promise((resolve, reject) => { + switch (filename) { + case 'jest.setup.ts': + return resolve("import './register';"); + case 'jest.setup.int.ts': + return resolve('export const foo = true;'); + default: + return reject(new Error(`Not implemented: ${filename}`)); + } + }), + ); + + await expect(tryAddEmptyExports('format')).resolves.toEqual({ + result: 'skip', + }); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('no-ops non-existent Jest setup files', async () => { + createDestinationFileReader.mockReturnValue(() => + Promise.resolve(undefined), + ); - it('no-ops non-existent Jest setup files', async () => { - createDestinationFileReader.mockReturnValue(() => - Promise.resolve(undefined), - ); + await expect(tryAddEmptyExports('format')).resolves.toEqual({ + result: 'skip', + }); - await expect(tryAddEmptyExports()).resolves.toBeUndefined(); + expect(writeFile).not.toHaveBeenCalled(); + }); - expect(writeFile).not.toHaveBeenCalled(); + it('logs and continues on internal failure', async () => { + const consoleLog = jest.spyOn(console, 'log').mockImplementation(); + + createDestinationFileReader.mockReturnValue(() => { + throw new Error('Something happened!'); + }); + + await expect(tryAddEmptyExports('format')).resolves.toEqual({ + result: 'skip', + reason: 'due to an error', + }); + + expect(writeFile).not.toHaveBeenCalled(); + + expect(consoleLog).toHaveBeenCalledTimes(2); + expect(consoleLog.mock.calls.flat()).toEqual([ + 'Failed to convert Jest setup files to isolated modules.', + expect.stringMatching(/^Error: Something happened!/), + ]); + }); }); - it('logs and continues on internal failure', async () => { - const consoleLog = jest.spyOn(console, 'log').mockImplementation(); + describe('lint mode', () => { + it('converts non-compliant Jest setup files', async () => { + createDestinationFileReader.mockReturnValue((filename) => + Promise.resolve(`// ${filename}`), + ); - createDestinationFileReader.mockReturnValue(() => { - throw new Error('Something happened!'); + await expect(tryAddEmptyExports('lint')).resolves.toEqual({ + result: 'apply', + }); + + expect(writeFile.mock.calls).toHaveLength(0); + }); + + it('no-ops compliant Jest setup files', async () => { + createDestinationFileReader.mockReturnValue( + (filename) => + new Promise((resolve, reject) => { + switch (filename) { + case 'jest.setup.ts': + return resolve("import './register';"); + case 'jest.setup.int.ts': + return resolve('export const foo = true;'); + default: + return reject(new Error(`Not implemented: ${filename}`)); + } + }), + ); + + await expect(tryAddEmptyExports('lint')).resolves.toEqual({ + result: 'skip', + }); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('no-ops non-existent Jest setup files', async () => { + createDestinationFileReader.mockReturnValue(() => + Promise.resolve(undefined), + ); + + await expect(tryAddEmptyExports('lint')).resolves.toEqual({ + result: 'skip', + }); + + expect(writeFile).not.toHaveBeenCalled(); }); - await expect(tryAddEmptyExports()).resolves.toBeUndefined(); + it('logs and continues on internal failure', async () => { + const consoleLog = jest.spyOn(console, 'log').mockImplementation(); + + createDestinationFileReader.mockReturnValue(() => { + throw new Error('Something happened!'); + }); - expect(writeFile).not.toHaveBeenCalled(); + await expect(tryAddEmptyExports('lint')).resolves.toEqual({ + result: 'skip', + reason: 'due to an error', + }); - expect(consoleLog).toHaveBeenCalledTimes(2); - expect(consoleLog.mock.calls.flat()).toEqual([ - 'Failed to convert Jest setup files to isolated modules.', - expect.stringMatching(/^Error: Something happened!/), - ]); + expect(writeFile).not.toHaveBeenCalled(); + + expect(consoleLog).toHaveBeenCalledTimes(2); + expect(consoleLog.mock.calls.flat()).toEqual([ + 'Failed to convert Jest setup files to isolated modules.', + expect.stringMatching(/^Error: Something happened!/), + ]); + }); }); }); diff --git a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts index 12c897b05..437577831 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/addEmptyExports.ts @@ -3,6 +3,7 @@ import { inspect } from 'util'; import fs from 'fs-extra'; +import type { PatchFunction } from '../..'; import { log } from '../../../../../utils/logging'; import { getDestinationManifest } from '../../../analysis/package'; import { createDestinationFileReader } from '../../../analysis/project'; @@ -10,7 +11,7 @@ import { formatPrettier } from '../../../processing/prettier'; const JEST_SETUP_FILES = ['jest.setup.ts', 'jest.setup.int.ts']; -const addEmptyExports = async () => { +const addEmptyExports = async (mode: 'format' | 'lint') => { const manifest = await getDestinationManifest(); const destinationRoot = path.dirname(manifest.path); @@ -28,7 +29,11 @@ const addEmptyExports = async () => { inputFile.includes('import ') || inputFile.includes('export ') ) { - return; + return 'skip'; + } + + if (mode === 'lint') { + return 'apply'; } const data = await formatPrettier([inputFile, 'export {}'].join('\n\n'), { @@ -38,20 +43,26 @@ const addEmptyExports = async () => { const filepath = path.join(destinationRoot, filename); await fs.promises.writeFile(filepath, data); + + return 'apply'; }; - await Promise.all(JEST_SETUP_FILES.map(addEmptyExport)); + const results = await Promise.all(JEST_SETUP_FILES.map(addEmptyExport)); + return results.every((result) => result === 'skip') ? 'skip' : 'apply'; }; /** * Tries to add an empty `export {}` statement to the bottom of Jest setup files * for compliance with TypeScript isolated modules. */ -export const tryAddEmptyExports = async () => { +export const tryAddEmptyExports: PatchFunction = async ( + mode: 'format' | 'lint', +) => { try { - await addEmptyExports(); + return { result: await addEmptyExports(mode) }; } catch (err) { log.warn('Failed to convert Jest setup files to isolated modules.'); log.subtle(inspect(err)); + return { result: 'skip', reason: 'due to an error' }; } }; diff --git a/src/cli/configure/upgrade/patches/7.3.1/index.ts b/src/cli/configure/upgrade/patches/7.3.1/index.ts index 314ef56f8..c303df123 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/index.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/index.ts @@ -1,14 +1,26 @@ +import type { Patches } from '../..'; import { tryPatchRenovateConfig } from '../../../patchRenovateConfig'; import { tryAddEmptyExports } from './addEmptyExports'; import { tryPatchDockerfile } from './patchDockerfile'; import { tryPatchServerListener } from './patchServerListener'; -export const upgrade = async () => { - await Promise.all([ - tryAddEmptyExports(), - tryPatchRenovateConfig(), - tryPatchDockerfile(), - tryPatchServerListener(), - ]); -}; +export const patches: Patches = [ + { + apply: tryAddEmptyExports, + description: + 'Add empty exports to Jest files for compliance with TypeScript isolated modules', + }, + { + apply: tryPatchRenovateConfig, + description: 'Update renovate config for private Renovate auth', + }, + { + apply: tryPatchDockerfile, + description: 'Swap node distroless Docker image to -debian11 variant', + }, + { + apply: tryPatchServerListener, + description: 'Add keepAliveTimeout to server listener', + }, +]; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts index a78198eac..7209cc189 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.test.ts @@ -18,27 +18,57 @@ FROM --platform=\${BUILDPLATFORM:-<%- platformName %>} gcr.io/distroless/nodejs: WORKDIR /workdir `; -it('patches an existing Dockerfile', async () => { - vol.fromJSON({ Dockerfile: dockerfile }); +describe('tryPatchDockerfile', () => { + describe('format mode', () => { + it('patches an existing Dockerfile', async () => { + vol.fromJSON({ Dockerfile: dockerfile }); - await expect(tryPatchDockerfile()).resolves.toBeUndefined(); + await expect(tryPatchDockerfile('format')).resolves.toEqual({ + result: 'apply', + }); - expect(volToJson()).toMatchInlineSnapshot(` -{ - "Dockerfile": " -ARG BASE_IMAGE -ARG BASE_TAG + expect(volToJson()).toMatchInlineSnapshot(` + { + "Dockerfile": " + ARG BASE_IMAGE + ARG BASE_TAG -FROM --platform=\${BUILDPLATFORM:-<%- platformName %>} gcr.io/distroless/nodejs18-debian11 AS runtime + FROM --platform=\${BUILDPLATFORM:-<%- platformName %>} gcr.io/distroless/nodejs18-debian11 AS runtime -WORKDIR /workdir -", -} -`); -}); + WORKDIR /workdir + ", + } + `); + }); + + it('ignores when a Dockerfile is missing', async () => { + vol.fromJSON({}); + + await expect(tryPatchDockerfile('format')).resolves.toEqual({ + result: 'skip', + reason: 'no Dockerfile found', + }); + }); + }); + + describe('lint mode', () => { + it('patches an existing Dockerfile', async () => { + vol.fromJSON({ Dockerfile: dockerfile }); + + await expect(tryPatchDockerfile('lint')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson().Dockerfile).toEqual(dockerfile); // unchanged + }); -it('ignores when a Dockerfile is missing', async () => { - vol.fromJSON({}); + it('ignores when a Dockerfile is missing', async () => { + vol.fromJSON({}); - await expect(tryPatchDockerfile()).resolves.toBeUndefined(); + await expect(tryPatchDockerfile('lint')).resolves.toEqual({ + result: 'skip', + reason: 'no Dockerfile found', + }); + }); + }); }); diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts index 6a265edf2..0b87c645f 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchDockerfile.ts @@ -2,6 +2,7 @@ import { inspect } from 'util'; import fs from 'fs-extra'; +import type { PatchFunction, PatchReturnType } from '../..'; import { log } from '../../../../../utils/logging'; import { createDestinationFileReader } from '../../../analysis/project'; @@ -10,13 +11,16 @@ const DOCKERFILE_FILENAME = 'Dockerfile'; const VERSION_REGEX = /gcr.io\/distroless\/nodejs:(16|18|20)/g; const VERSION_DEBIAN_REPLACE = 'gcr.io/distroless/nodejs$1-debian11'; -const patchDockerfile = async (dir: string) => { +const patchDockerfile = async ( + mode: 'format' | 'lint', + dir: string, +): Promise => { const readFile = createDestinationFileReader(dir); const maybeDockerfile = await readFile(DOCKERFILE_FILENAME); if (!maybeDockerfile) { - return; + return { result: 'skip', reason: 'no Dockerfile found' }; } const patched = maybeDockerfile.replaceAll( @@ -24,14 +28,28 @@ const patchDockerfile = async (dir: string) => { VERSION_DEBIAN_REPLACE, ); + if (patched === maybeDockerfile) { + return { result: 'skip' }; + } + + if (mode === 'lint') { + return { result: 'apply' }; + } + await fs.promises.writeFile(DOCKERFILE_FILENAME, patched); + + return { result: 'apply' }; }; -export const tryPatchDockerfile = async (dir = process.cwd()) => { +export const tryPatchDockerfile: PatchFunction = async ( + mode: 'format' | 'lint', + dir = process.cwd(), +) => { try { - await patchDockerfile(dir); + return await patchDockerfile(mode, dir); } catch (err) { log.warn('Failed to patch Dockerfile.'); log.subtle(inspect(err)); + return { result: 'skip', reason: 'due to an error' }; } }; diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts index 1ce37b77e..677dec633 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.test.ts @@ -30,147 +30,306 @@ const volToJson = () => vol.toJSON(process.cwd(), undefined, true); beforeEach(jest.clearAllMocks); beforeEach(() => vol.reset()); -it('patches a listener with a callback and existing variable reference', async () => { - vol.fromJSON({ 'src/listen.ts': LISTENER_WITH_CALLBACK }); - - await expect(tryPatchServerListener()).resolves.toBeUndefined(); - - expect(volToJson()).toMatchInlineSnapshot(` - { - "src/listen.ts": "const listener = app.listen(config.port, () => { - const address = listener.address(); - - if (typeof address === 'object' && address) { - logger.debug(\`listening on port \${address.port}\`); - } - }); - - // Gantry ALB default idle timeout is 30 seconds - // https://nodejs.org/docs/latest-v18.x/api/http.html#serverkeepalivetimeout - // Node default is 5 seconds - // https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#connection-idle-timeout - // AWS recommends setting an application timeout larger than the load balancer - listener.keepAliveTimeout = 31000; - ", - } - `); -}); +describe('patchServerListener', () => { + describe('format mode', () => { + it('patches a listener with a callback and existing variable reference', async () => { + vol.fromJSON({ 'src/listen.ts': LISTENER_WITH_CALLBACK }); + + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toMatchInlineSnapshot(` + { + "src/listen.ts": "const listener = app.listen(config.port, () => { + const address = listener.address(); + + if (typeof address === 'object' && address) { + logger.debug(\`listening on port \${address.port}\`); + } + }); + + // Gantry ALB default idle timeout is 30 seconds + // https://nodejs.org/docs/latest-v18.x/api/http.html#serverkeepalivetimeout + // Node default is 5 seconds + // https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#connection-idle-timeout + // AWS recommends setting an application timeout larger than the load balancer + listener.keepAliveTimeout = 31000; + ", + } + `); + }); -it('patches a listener without a callback', async () => { - vol.fromJSON({ 'src/listen.ts': LISTENER_WITHOUT_CALLBACK }); + it('patches a listener without a callback', async () => { + vol.fromJSON({ 'src/listen.ts': LISTENER_WITHOUT_CALLBACK }); + + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toMatchInlineSnapshot(` + { + "src/listen.ts": "const listener = app.listen(config.port); + + // Gantry ALB default idle timeout is 30 seconds + // https://nodejs.org/docs/latest-v18.x/api/http.html#serverkeepalivetimeout + // Node default is 5 seconds + // https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#connection-idle-timeout + // AWS recommends setting an application timeout larger than the load balancer + listener.keepAliveTimeout = 31000; + ", + } + `); + }); - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + it('handles a lack of server listener', async () => { + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'no listener file found', + }); - expect(volToJson()).toMatchInlineSnapshot(` - { - "src/listen.ts": "const listener = app.listen(config.port); + expect(volToJson()).toStrictEqual({}); + }); - // Gantry ALB default idle timeout is 30 seconds - // https://nodejs.org/docs/latest-v18.x/api/http.html#serverkeepalivetimeout - // Node default is 5 seconds - // https://docs.aws.amazon.com/elasticloadbalancing/latest/application/application-load-balancers.html#connection-idle-timeout - // AWS recommends setting an application timeout larger than the load balancer - listener.keepAliveTimeout = 31000; - ", - } - `); -}); + it('handles a filesystem error', async () => { + const err = new Error('Badness!'); -it('handles a lack of server listener', async () => { - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + writeFile.mockRejectedValueOnce(err); - expect(volToJson()).toStrictEqual({}); -}); + const files = { 'src/listen.ts': LISTENER_WITH_CALLBACK }; -it('handles a filesystem error', async () => { - const err = new Error('Badness!'); + vol.fromJSON(files); - writeFile.mockRejectedValueOnce(err); + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'due to an error', + }); - const files = { 'src/listen.ts': LISTENER_WITH_CALLBACK }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(consoleLog).toHaveBeenCalledWith( + 'Failed to patch server listener.', + ); + expect(consoleLog).toHaveBeenCalledWith(inspect(err)); + }); - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + it('skips the templated Koa listener', async () => { + const listener = await fs.promises.readFile( + require.resolve( + '../../../../../../template/koa-rest-api/src/listen.ts', + ), + 'utf-8', + ); - expect(volToJson()).toStrictEqual(files); + const files = { 'src/listen.ts': listener }; - expect(consoleLog).toHaveBeenCalledWith('Failed to patch server listener.'); - expect(consoleLog).toHaveBeenCalledWith(inspect(err)); -}); + vol.fromJSON(files); -it('skips the templated Koa listener', async () => { - const listener = await fs.promises.readFile( - require.resolve('../../../../../../template/koa-rest-api/src/listen.ts'), - 'utf-8', - ); + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); - const files = { 'src/listen.ts': listener }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + it('skips the templated Express listener', async () => { + const listener = await fs.promises.readFile( + require.resolve( + '../../../../../../template/express-rest-api/src/listen.ts', + ), + 'utf-8', + ); - expect(volToJson()).toStrictEqual(files); + const files = { 'src/listen.ts': listener }; - expect(writeFile).not.toHaveBeenCalled(); -}); + vol.fromJSON(files); -it('skips the templated Express listener', async () => { - const listener = await fs.promises.readFile( - require.resolve( - '../../../../../../template/express-rest-api/src/listen.ts', - ), - 'utf-8', - ); + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); - const files = { 'src/listen.ts': listener }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + it('skips a file with keep-alive already set', async () => { + const listener = `${LISTENER_WITH_CALLBACK} - expect(volToJson()).toStrictEqual(files); + listener.keepAliveTimeout = 0;`; - expect(writeFile).not.toHaveBeenCalled(); -}); + const files = { 'src/listen.ts': listener }; -it('skips a file with keep-alive already set', async () => { - const listener = `${LISTENER_WITH_CALLBACK} + vol.fromJSON(files); -listener.keepAliveTimeout = 0;`; + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); - const files = { 'src/listen.ts': listener }; + expect(volToJson()).toStrictEqual(files); - vol.fromJSON(files); + expect(writeFile).not.toHaveBeenCalled(); + }); - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + it('skips a non-standard file', async () => { + const files = { 'src/listen.ts': "console.log('Who dis?')" }; - expect(volToJson()).toStrictEqual(files); + vol.fromJSON(files); - expect(writeFile).not.toHaveBeenCalled(); -}); + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'no server listener found', + }); + + expect(volToJson()).toStrictEqual(files); -it('skips a non-standard file', async () => { - const files = { 'src/listen.ts': "console.log('Who dis?')" }; + expect(writeFile).not.toHaveBeenCalled(); + }); - vol.fromJSON(files); + it('skips an empty file', async () => { + const files = { 'src/listen.ts': '' }; - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + vol.fromJSON(files); - expect(volToJson()).toStrictEqual(files); + await expect(tryPatchServerListener('format')).resolves.toEqual({ + result: 'skip', + reason: 'no listener file found', + }); - expect(writeFile).not.toHaveBeenCalled(); -}); + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + }); + + describe('lint mode', () => { + it('patches a listener with a callback and existing variable reference', async () => { + vol.fromJSON({ 'src/listen.ts': LISTENER_WITH_CALLBACK }); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toEqual({ 'src/listen.ts': LISTENER_WITH_CALLBACK }); + }); + + it('patches a listener without a callback', async () => { + vol.fromJSON({ 'src/listen.ts': LISTENER_WITHOUT_CALLBACK }); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'apply', + }); + + expect(volToJson()).toEqual({ + 'src/listen.ts': LISTENER_WITHOUT_CALLBACK, + }); + }); + + it('handles a lack of server listener', async () => { + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'no listener file found', + }); + + expect(volToJson()).toStrictEqual({}); + }); -it('skips an empty file', async () => { - const files = { 'src/listen.ts': '' }; + it('skips the templated Koa listener', async () => { + const listener = await fs.promises.readFile( + require.resolve( + '../../../../../../template/koa-rest-api/src/listen.ts', + ), + 'utf-8', + ); - vol.fromJSON(files); + const files = { 'src/listen.ts': listener }; - await expect(tryPatchServerListener()).resolves.toBeUndefined(); + vol.fromJSON(files); - expect(volToJson()).toStrictEqual(files); + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); - expect(writeFile).not.toHaveBeenCalled(); + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips the templated Express listener', async () => { + const listener = await fs.promises.readFile( + require.resolve( + '../../../../../../template/express-rest-api/src/listen.ts', + ), + 'utf-8', + ); + + const files = { 'src/listen.ts': listener }; + + vol.fromJSON(files); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a file with keep-alive already set', async () => { + const listener = `${LISTENER_WITH_CALLBACK} + + listener.keepAliveTimeout = 0;`; + + const files = { 'src/listen.ts': listener }; + + vol.fromJSON(files); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'keepAliveTimeout already configured', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips a non-standard file', async () => { + const files = { 'src/listen.ts': "console.log('Who dis?')" }; + + vol.fromJSON(files); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'no server listener found', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + + it('skips an empty file', async () => { + const files = { 'src/listen.ts': '' }; + + vol.fromJSON(files); + + await expect(tryPatchServerListener('lint')).resolves.toEqual({ + result: 'skip', + reason: 'no listener file found', + }); + + expect(volToJson()).toStrictEqual(files); + + expect(writeFile).not.toHaveBeenCalled(); + }); + }); }); diff --git a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts index 8491f19a4..12727a78e 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/patchServerListener.ts @@ -2,6 +2,7 @@ import { inspect } from 'util'; import fs from 'fs-extra'; +import type { PatchFunction, PatchReturnType } from '../..'; import { log } from '../../../../../utils/logging'; import { createDestinationFileReader } from '../../../analysis/project'; import { formatPrettier } from '../../../processing/prettier'; @@ -17,13 +18,19 @@ const KEEP_ALIVE_CODE = ` listener.keepAliveTimeout = 31000; `; -const patchServerListener = async (dir: string) => { +const patchServerListener = async ( + mode: 'format' | 'lint', + dir: string, +): Promise => { const readFile = createDestinationFileReader(dir); let listener = await readFile(SERVER_LISTENER_FILENAME); + if (!listener) { + return { result: 'skip', reason: 'no listener file found' }; + } - if (!listener || listener.includes('keepAliveTimeout')) { - return; + if (listener.includes('keepAliveTimeout')) { + return { result: 'skip', reason: 'keepAliveTimeout already configured' }; } if (listener.includes('\napp.listen(')) { @@ -34,7 +41,11 @@ const patchServerListener = async (dir: string) => { } if (!listener.includes('\nconst listener = app.listen(')) { - return; + return { result: 'skip', reason: 'no server listener found' }; + } + + if (mode === 'lint') { + return { result: 'apply' }; } listener = `${listener}${KEEP_ALIVE_CODE}`; @@ -45,13 +56,19 @@ const patchServerListener = async (dir: string) => { parser: 'typescript', }), ); + + return { result: 'apply' }; }; -export const tryPatchServerListener = async (dir = process.cwd()) => { +export const tryPatchServerListener: PatchFunction = async ( + mode: 'format' | 'lint', + dir = process.cwd(), +) => { try { - await patchServerListener(dir); + return await patchServerListener(mode, dir); } catch (err) { log.warn('Failed to patch server listener.'); log.subtle(inspect(err)); + return { result: 'skip', reason: 'due to an error' }; } }; diff --git a/src/cli/init/index.ts b/src/cli/init/index.ts index dd01a6121..dfd438bb0 100644 --- a/src/cli/init/index.ts +++ b/src/cli/init/index.ts @@ -88,7 +88,7 @@ export const init = async (args = process.argv.slice(2)) => { await initialiseRepo(destinationDir, templateData); // Patch in a baseline Renovate preset based on the configured Git owner. - await tryPatchRenovateConfig(destinationDir); + await tryPatchRenovateConfig('format', destinationDir); const skubaSlug = `skuba@${skubaVersionInfo.local}`; diff --git a/src/cli/lint.int.test.ts b/src/cli/lint.int.test.ts index 8165e5d08..ff39a82fc 100644 --- a/src/cli/lint.int.test.ts +++ b/src/cli/lint.int.test.ts @@ -120,7 +120,7 @@ test.each` ${'ok'} | ${[]} | ${'ok'} | ${'0.0.0'} | ${undefined} ${'ok --debug'} | ${['--debug']} | ${'ok'} | ${'0.0.0'} | ${undefined} ${'unfixable'} | ${[]} | ${'unfixable'} | ${'0.0.0'} | ${1} - ${'needs patches'} | ${[]} | ${'ok'} | ${'1.0.0'} | ${1} + ${'needs patches'} | ${[]} | ${'patch'} | ${'1.0.0'} | ${1} `('$description', async ({ args, base, skubaVersion, exitCode }: Args) => { jest.mocked(getSkubaVersion).mockResolvedValue(skubaVersion); From b2a6b6e641c215fe3747e51adcbb46ac14f571b0 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:13:24 +1100 Subject: [PATCH 07/13] Wording updates --- src/cli/__snapshots__/format.int.test.ts.snap | 16 ++++++------- src/cli/configure/patchRenovateConfig.test.ts | 24 +++++++++---------- src/cli/configure/patchRenovateConfig.ts | 8 +++---- .../configure/upgrade/patches/7.3.1/index.ts | 4 ++-- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/cli/__snapshots__/format.int.test.ts.snap b/src/cli/__snapshots__/format.int.test.ts.snap index 38fa0e33f..59c6a545f 100644 --- a/src/cli/__snapshots__/format.int.test.ts.snap +++ b/src/cli/__snapshots__/format.int.test.ts.snap @@ -10,9 +10,9 @@ Updating skuba... Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules -Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config +Patch skipped: Update Renovate config to support private SEEK packages - owner does not map to a SEEK preset -Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found +Patch skipped: Upgrade Node.js Distroless Docker image to -debian11 variant - no Dockerfile found Patch skipped: Add keepAliveTimeout to server listener - no listener file found @@ -77,9 +77,9 @@ Updating skuba... Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules -Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config +Patch skipped: Update Renovate config to support private SEEK packages - owner does not map to a SEEK preset -Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found +Patch skipped: Upgrade Node.js Distroless Docker image to -debian11 variant - no Dockerfile found Patch skipped: Add keepAliveTimeout to server listener - no listener file found @@ -137,9 +137,9 @@ Updating skuba... Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules -Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config +Patch skipped: Update Renovate config to support private SEEK packages - owner does not map to a SEEK preset -Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found +Patch skipped: Upgrade Node.js Distroless Docker image to -debian11 variant - no Dockerfile found Patch skipped: Add keepAliveTimeout to server listener - no listener file found @@ -170,9 +170,9 @@ Updating skuba... Patch skipped: Add empty exports to Jest files for compliance with TypeScript isolated modules -Patch skipped: Update renovate config for private Renovate auth - owner does not map to any of SEEK's renovate auth config +Patch skipped: Update Renovate config to support private SEEK packages - owner does not map to a SEEK preset -Patch skipped: Swap node distroless Docker image to -debian11 variant - no Dockerfile found +Patch skipped: Upgrade Node.js Distroless Docker image to -debian11 variant - no Dockerfile found Patch skipped: Add keepAliveTimeout to server listener - no listener file found diff --git a/src/cli/configure/patchRenovateConfig.test.ts b/src/cli/configure/patchRenovateConfig.test.ts index 4c554a2f6..db15e106b 100644 --- a/src/cli/configure/patchRenovateConfig.test.ts +++ b/src/cli/configure/patchRenovateConfig.test.ts @@ -139,7 +139,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: 'no renovate config found', + reason: 'no config found', }); expect(volToJson()).toStrictEqual(files); @@ -183,7 +183,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: 'could not find git root', + reason: 'no Git root found', }); expect(volToJson()).toStrictEqual(files); @@ -200,7 +200,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: "owner does not map to any of SEEK's renovate auth config", + reason: 'owner does not map to a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -220,7 +220,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: "owner does not map to any of SEEK's renovate auth config", + reason: 'owner does not map to a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -262,7 +262,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: 'renovate config already has private auth', + reason: 'config already has a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -282,7 +282,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('format')).resolves.toEqual({ result: 'skip', - reason: 'renovate config already has private auth', + reason: 'config already has a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -356,7 +356,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: 'no renovate config found', + reason: 'no config found', }); expect(volToJson()).toStrictEqual(files); @@ -373,7 +373,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: 'could not find git root', + reason: 'no Git root found', }); expect(volToJson()).toStrictEqual(files); @@ -390,7 +390,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: "owner does not map to any of SEEK's renovate auth config", + reason: 'owner does not map to a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -410,7 +410,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: "owner does not map to any of SEEK's renovate auth config", + reason: 'owner does not map to a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -452,7 +452,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: 'renovate config already has private auth', + reason: 'config already has a SEEK preset', }); expect(volToJson()).toStrictEqual(files); @@ -472,7 +472,7 @@ describe('patchRenovateConfig', () => { await expect(tryPatchRenovateConfig('lint')).resolves.toEqual({ result: 'skip', - reason: 'renovate config already has private auth', + reason: 'config already has a SEEK preset', }); expect(volToJson()).toStrictEqual(files); diff --git a/src/cli/configure/patchRenovateConfig.ts b/src/cli/configure/patchRenovateConfig.ts index 6d8a3645f..57c97c1e6 100644 --- a/src/cli/configure/patchRenovateConfig.ts +++ b/src/cli/configure/patchRenovateConfig.ts @@ -105,7 +105,7 @@ const patchRenovateConfig = async ( if (!presetToAdd) { return { result: 'skip', - reason: "owner does not map to any of SEEK's renovate auth config", + reason: 'owner does not map to a SEEK preset', }; } @@ -118,7 +118,7 @@ const patchRenovateConfig = async ( const config = maybeConfigs.find((maybeConfig) => Boolean(maybeConfig.input)); if (!config?.input) { - return { result: 'skip', reason: 'no renovate config found' }; + return { result: 'skip', reason: 'no config found' }; } if ( @@ -131,7 +131,7 @@ const patchRenovateConfig = async ( ) { return { result: 'skip', - reason: 'renovate config already has private auth', + reason: 'config already has a SEEK preset', }; } @@ -165,7 +165,7 @@ export const tryPatchRenovateConfig = (async ( // with Renovate config that should be relative to the repository root. const gitRoot = await Git.findRoot({ dir }); if (!gitRoot) { - return { result: 'skip', reason: 'could not find git root' }; + return { result: 'skip', reason: 'no Git root found' }; } return await patchRenovateConfig(mode, gitRoot); diff --git a/src/cli/configure/upgrade/patches/7.3.1/index.ts b/src/cli/configure/upgrade/patches/7.3.1/index.ts index c303df123..b0f815f8a 100644 --- a/src/cli/configure/upgrade/patches/7.3.1/index.ts +++ b/src/cli/configure/upgrade/patches/7.3.1/index.ts @@ -13,11 +13,11 @@ export const patches: Patches = [ }, { apply: tryPatchRenovateConfig, - description: 'Update renovate config for private Renovate auth', + description: 'Update Renovate config to support private SEEK packages', }, { apply: tryPatchDockerfile, - description: 'Swap node distroless Docker image to -debian11 variant', + description: 'Upgrade Node.js Distroless Docker image to -debian11 variant', }, { apply: tryPatchServerListener, From 42035a04c5b65100a2268f61c8967915d2d93684 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:15:37 +1100 Subject: [PATCH 08/13] Autofix spruik --- .changeset/five-dancers-pump.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.changeset/five-dancers-pump.md b/.changeset/five-dancers-pump.md index c04f583fb..53739aa2e 100644 --- a/.changeset/five-dancers-pump.md +++ b/.changeset/five-dancers-pump.md @@ -10,3 +10,7 @@ This fixes issues where skuba would not fail a `lint` check but silently make ch These changes may never end up being committed and causes noise when running `lint` or `format` later. Now, lints report whether changes need to be made and are applied in `format` or autofix modes (in CI). + +In addition, `skuba lint` can now automatically push autofixes. This eases adoption of linting rule changes and automatically resolves issues arising from a forgotten `skuba format`. + +You'll need to configure your CI environment to support this feature. See our [GitHub autofixes](https://seek-oss.github.io/skuba/docs/deep-dives/github.html#github-autofixes) documentation to learn more. From c93b65319abce21071c26c769f88c00d478f2d9e Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:16:13 +1100 Subject: [PATCH 09/13] Update src/cli/configure/patchRenovateConfig.ts Co-authored-by: Ryan Ling --- src/cli/configure/patchRenovateConfig.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cli/configure/patchRenovateConfig.ts b/src/cli/configure/patchRenovateConfig.ts index 57c97c1e6..771710c15 100644 --- a/src/cli/configure/patchRenovateConfig.ts +++ b/src/cli/configure/patchRenovateConfig.ts @@ -123,8 +123,7 @@ const patchRenovateConfig = async ( if ( // The file appears to mention the baseline preset for the configured Git - // owner. This is a very naive check that we don't want to overcomplicate - // because it is invoked before each skuba format and lint. + // owner. This is a naive check for simplicity. config.input.includes(presetToAdd) || // Ignore any renovate configuration which already extends a SEEK-Jobs or seekasia config EXISTING_REPO_PRESET_REGEX.exec(config.input) From 72e34853889c9e7ea4cb5cc9174e3f4442e2a99b Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:17:39 +1100 Subject: [PATCH 10/13] Update src/cli/format.ts Co-authored-by: Ryan Ling --- src/cli/format.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cli/format.ts b/src/cli/format.ts index 8ababfd76..2e3ecb902 100644 --- a/src/cli/format.ts +++ b/src/cli/format.ts @@ -8,10 +8,11 @@ import { runPrettier } from './adapter/prettier'; import { internalLint } from './lint/internal'; export const format = async (args = process.argv.slice(2)): Promise => { + const debug = hasDebugFlag(args); + log.plain(chalk.blueBright('skuba lints')); - const internal = await internalLint('format', { debug: false, serial: true }); + const internal = await internalLint('format', { debug, serial: true }); - const debug = hasDebugFlag(args); const logger = createLogger(debug); log.newline(); From 95ecfe4ab768238fe612009610f3c6616432376d Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:18:35 +1100 Subject: [PATCH 11/13] Update .changeset/five-dancers-pump.md Co-authored-by: Ryan Ling --- .changeset/five-dancers-pump.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/.changeset/five-dancers-pump.md b/.changeset/five-dancers-pump.md index 53739aa2e..31c4118ac 100644 --- a/.changeset/five-dancers-pump.md +++ b/.changeset/five-dancers-pump.md @@ -2,14 +2,13 @@ 'skuba': minor --- -lint: Overhaul skuba's internal linting system +lint: Overhaul internal linting system -Internal (skuba) linting is now promoted to a top-level tool alongside ESLint, tsc, and Prettier. +Internal linting is now promoted to a top-level tool alongside ESLint, Prettier, and tsc. -This fixes issues where skuba would not fail a `lint` check but silently make changes. -These changes may never end up being committed and causes noise when running `lint` or `format` later. +This fixes issues where skuba would not fail a `skuba lint` check but silently make changes to your working tree. These changes may have never been committed and subsequently led to noise when running `skuba format` or `skuba lint`. -Now, lints report whether changes need to be made and are applied in `format` or autofix modes (in CI). +Now, internal linting rules report whether changes need to be made, and changes are only applied in `format` or autofix modes (in CI). In addition, `skuba lint` can now automatically push autofixes. This eases adoption of linting rule changes and automatically resolves issues arising from a forgotten `skuba format`. From 80f98de4c507011f13204d434546daa93d37db01 Mon Sep 17 00:00:00 2001 From: Aaron Moat <2937187+AaronMoat@users.noreply.github.com> Date: Wed, 10 Jan 2024 11:24:38 +1100 Subject: [PATCH 12/13] Clean up double message --- .../internalLints/refreshIgnoreFiles.test.ts | 4 +-- .../lint/internalLints/refreshIgnoreFiles.ts | 32 +++++++++---------- 2 files changed, 17 insertions(+), 19 deletions(-) diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts index c9937a5a8..44b748ec0 100644 --- a/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.test.ts @@ -94,8 +94,8 @@ describe('refreshIgnoreFiles', () => { }); expect(`\n${stdout()}`).toBe(` -The .eslintignore file is out of date. Run pnpm exec skuba format to update it. refresh-ignore-files -The .gitignore file is out of date. Run pnpm exec skuba format to update it. refresh-ignore-files +The .eslintignore file is out of date. Run \`pnpm exec skuba format\` to update it. refresh-ignore-files +The .gitignore file is out of date. Run \`pnpm exec skuba format\` to update it. refresh-ignore-files `); expect(writeFile).not.toHaveBeenCalled(); diff --git a/src/cli/lint/internalLints/refreshIgnoreFiles.ts b/src/cli/lint/internalLints/refreshIgnoreFiles.ts index 1171d54a8..6de93d145 100644 --- a/src/cli/lint/internalLints/refreshIgnoreFiles.ts +++ b/src/cli/lint/internalLints/refreshIgnoreFiles.ts @@ -2,6 +2,7 @@ import path from 'path'; import { inspect } from 'util'; import { writeFile } from 'fs-extra'; +import stripAnsi from 'strip-ansi'; import type { Logger } from '../../../utils/logging'; import { detectPackageManager } from '../../../utils/packageManager'; @@ -53,9 +54,8 @@ export const refreshIgnoreFiles = async ( await writeFile(filepath, data); return { needsChange: false, - msg: `Refreshed ${logger.bold(filename)}. ${logger.dim( - 'refresh-ignore-files', - )}`, + msg: `Refreshed ${logger.bold(filename)}.`, + filename, }; } @@ -66,13 +66,12 @@ export const refreshIgnoreFiles = async ( needsChange: true, msg: `The ${logger.bold( filename, - )} file is out of date. Run ${logger.bold( + )} file is out of date. Run \`${logger.bold( packageManager.exec, 'skuba', 'format', - )} to update it. ${logger.dim('refresh-ignore-files')}`, + )}\` to update it.`, filename, - annotationMessage: `The ${filename} file is out of date. Run \`${packageManager.exec} skuba format\` to update it.`, }; } @@ -86,7 +85,7 @@ export const refreshIgnoreFiles = async ( // Log after for reproducible test output ordering results.forEach((result) => { if (result.msg) { - logger.warn(result.msg); + logger.warn(result.msg, logger.dim('refresh-ignore-files')); } }); @@ -95,16 +94,15 @@ export const refreshIgnoreFiles = async ( return { ok: !anyNeedChanging, fixable: anyNeedChanging, - annotations: results.flatMap( - ({ needsChange, filename, annotationMessage }) => - needsChange && annotationMessage - ? [ - { - path: filename, - message: annotationMessage, - }, - ] - : [], + annotations: results.flatMap(({ needsChange, filename, msg }) => + needsChange && msg + ? [ + { + path: filename, + message: stripAnsi(msg), + }, + ] + : [], ), }; }; From 6a10c00c111a806bc6937cd7e7bb2b8390dc8942 Mon Sep 17 00:00:00 2001 From: Ryan Ling Date: Wed, 10 Jan 2024 12:33:33 +1100 Subject: [PATCH 13/13] Update five-dancers-pump.md --- .changeset/five-dancers-pump.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/.changeset/five-dancers-pump.md b/.changeset/five-dancers-pump.md index 31c4118ac..d176e210e 100644 --- a/.changeset/five-dancers-pump.md +++ b/.changeset/five-dancers-pump.md @@ -4,12 +4,8 @@ lint: Overhaul internal linting system -Internal linting is now promoted to a top-level tool alongside ESLint, Prettier, and tsc. +Previously, internal lint rules would not fail a `skuba lint` check but would silently make changes to your working tree. These changes may have never been committed and may have caused subsequent noise when running `skuba format` or `skuba lint`. -This fixes issues where skuba would not fail a `skuba lint` check but silently make changes to your working tree. These changes may have never been committed and subsequently led to noise when running `skuba format` or `skuba lint`. +Now, internal linting is now promoted to a top-level tool alongside ESLint, Prettier, and tsc. Rules will report whether changes need to be made, and changes will only be applied in `format` or autofix modes (in CI). As a consequence, `skuba lint` may fail upon upgrading to this version if your project has internal lint violations that have been left unaddressed up to this point. -Now, internal linting rules report whether changes need to be made, and changes are only applied in `format` or autofix modes (in CI). - -In addition, `skuba lint` can now automatically push autofixes. This eases adoption of linting rule changes and automatically resolves issues arising from a forgotten `skuba format`. - -You'll need to configure your CI environment to support this feature. See our [GitHub autofixes](https://seek-oss.github.io/skuba/docs/deep-dives/github.html#github-autofixes) documentation to learn more. +You can configure `skuba lint` to automatically push autofixes; this eases adoption of linting rule changes and automatically resolves issues arising from a forgotten `skuba format`. You'll need to configure your CI environment to support this feature. See our [GitHub autofixes](https://seek-oss.github.io/skuba/docs/deep-dives/github.html#github-autofixes) documentation to learn more.