Skip to content

Add GitHub Actions reporter #1058

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add GitHub Actions reporter #1058

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e536e39
Add reporter from @arthurfiorette
EzraBrooks Apr 28, 2025
8a881b4
Cleanup and test
EzraBrooks Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions bun.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,24 @@
"type-fest": "^4.31.0",
"typescript": "^5.5.2",
},
"optionalDependencies": {
"@actions/core": "^1.11.1",
},
"peerDependencies": {
"@types/node": ">=18",
"typescript": ">=5.0.4",
},
},
},
"packages": {
"@actions/core": ["@actions/[email protected]", "", { "dependencies": { "@actions/exec": "^1.1.1", "@actions/http-client": "^2.0.1" } }, "sha512-hXJCSrkwfA46Vd9Z3q4cpEpHB1rL5NG04+/rbqW9d3+CSvtB1tYe8UTpAlixa1vj0m/ULglfEK2UKxMGxCxv5A=="],

"@actions/exec": ["@actions/[email protected]", "", { "dependencies": { "@actions/io": "^1.0.1" } }, "sha512-+sCcHHbVdk93a0XT19ECtO/gIXoxvdsgQLzb2fE2/5sIZmWQuluYyjPQtrtTHdU1YzTZ7bAPN4sITq2xi1679w=="],

"@actions/http-client": ["@actions/[email protected]", "", { "dependencies": { "tunnel": "^0.0.6", "undici": "^5.25.4" } }, "sha512-mx8hyJi/hjFvbPokCg4uRd4ZX78t+YyRPtnKWwIl+RzNaVuFpQHfmlGVfsKEJN8LwTCvL+DfVgAM04XaHkm6bA=="],

"@actions/io": ["@actions/[email protected]", "", {}, "sha512-wi9JjgKLYS7U/z8PPbco+PvTb/nRWjeoFlJ1Qer83k/3C5PHQi28hiVdeE2kHXmIL99mQFawx8qt/JPjZilJ8Q=="],

"@astro-community/astro-embed-youtube": ["@astro-community/[email protected]", "", { "dependencies": { "lite-youtube-embed": "^0.3.3" }, "peerDependencies": { "astro": "^2.0.0 || ^3.0.0-beta || ^4.0.0-beta || ^5.0.0-beta" } }, "sha512-/mRfCl/eTBUz0kmjD1psOy0qoDDBorVp0QumUacjFcIkBullYtbeFQ2ZGZ+3N/tA6cR/OIyzr2QA4dQXlY6USg=="],

"@astrojs/check": ["@astrojs/[email protected]", "", { "dependencies": { "@astrojs/language-server": "^2.15.0", "chokidar": "^4.0.1", "kleur": "^4.1.5", "yargs": "^17.7.2" }, "peerDependencies": { "typescript": "^5.0.0" }, "bin": { "astro-check": "dist/bin.js" } }, "sha512-IOheHwCtpUfvogHHsvu0AbeRZEnjJg3MopdLddkJE70mULItS/Vh37BHcI00mcOJcH1vhD3odbpvWokpxam7xA=="],
Expand Down Expand Up @@ -219,6 +230,8 @@

"@expressive-code/plugin-text-markers": ["@expressive-code/[email protected]", "", { "dependencies": { "@expressive-code/core": "^0.41.1" } }, "sha512-PFvk91yY+H8KVEcyZSrktLoWzBgLVpowvMxOJooFn74roGxnU4TEBJpWcRnJFtMEwTLzWNnk10MSOApOccvSKg=="],

"@fastify/busboy": ["@fastify/[email protected]", "", {}, "sha512-vBZP4NlzfOlerQTnba4aqZoMhE/a9HY7HRqoOPaETQcSQuWEIyZMHGfVu6w9wGtGK5fED5qRs2DteVCjOH60sA=="],

"@iarna/toml": ["@iarna/[email protected]", "", {}, "sha512-td6ZUkz2oS3VeleBcN+m//Q6HlCFCPrnI0FZhrt/h4XqLEdOyYp2u21nd8MdsR+WJy5r9PTDaHTDDfhf4H4l6Q=="],

"@img/sharp-darwin-arm64": ["@img/[email protected]", "", { "optionalDependencies": { "@img/sharp-libvips-darwin-arm64": "1.1.0" }, "os": "darwin", "cpu": "arm64" }, "sha512-pn44xgBtgpEbZsu+lWf2KNb6OAf70X68k+yk69Ic2Xz11zHR/w24/U49XT7AeRwJ0Px+mhALhU5LPci1Aymk7A=="],
Expand Down Expand Up @@ -1627,6 +1640,8 @@

"tslib": ["[email protected]", "", {}, "sha512-oJFu94HQb+KVduSUQL7wnpmqnfmLsOA/nAh6b6EH0wCEoK0/mPeXU6c3wKDV83MkOuHPRHtSXKKU99IBazS/2w=="],

"tunnel": ["[email protected]", "", {}, "sha512-1h/Lnq9yajKY2PEbBadPXj3VxsDDu844OnaAo52UVmIzIvwwtBPIuNvkjuzBlTWpfJyUbG3ez0KSBibQkj4ojg=="],

"type-fest": ["[email protected]", "", {}, "sha512-S/5/0kFftkq27FPNye0XM1e2NsnoD/3FS+pBmbjmmtLT6I+i344KoOf7pvXreaFsDamWeaJX55nczA1m5PsBDg=="],

"typedarray": ["[email protected]", "", {}, "sha512-/aCDEGatGvZ2BIk+HmLf4ifCJFwvKFNb9/JeZPMulfgFracn9QFcAf5GO8B/mweUjSoblS5In0cWhqpfs/5PQA=="],
Expand Down Expand Up @@ -1819,6 +1834,8 @@

"zwitch": ["[email protected]", "", {}, "sha512-bXE4cR/kVZhKZX/RjPEflHaKVhUVl85noU3v6b8apfQEc1x4A+zBxjZ4lN8LqGd6WZ3dl98pY4o717VFmoPp+A=="],

"@actions/http-client/undici": ["[email protected]", "", { "dependencies": { "@fastify/busboy": "^2.0.0" } }, "sha512-raqeBD6NQK4SkWhQzeYKd1KmIG6dllBOTt55Rmkt4HtI9mwdWtJljnrXjAFUBLTSN67HWrOIZ3EPF4kjUw80Bg=="],

"@astrojs/starlight/remark-directive": ["[email protected]", "", { "dependencies": { "@types/mdast": "^4.0.0", "mdast-util-directive": "^3.0.0", "micromark-extension-directive": "^3.0.0", "unified": "^11.0.0" } }, "sha512-gwglrEQEZcZYgVyG1tQuA+h58EZfq5CSULw7J90AFuCTyib1thgHPoqQ+h9iFvU6R+vnZ5oNFQR5QKgGpk741A=="],

"@expressive-code/plugin-shiki/shiki": ["[email protected]", "", { "dependencies": { "@shikijs/core": "3.2.2", "@shikijs/engine-javascript": "3.2.2", "@shikijs/engine-oniguruma": "3.2.2", "@shikijs/langs": "3.2.2", "@shikijs/themes": "3.2.2", "@shikijs/types": "3.2.2", "@shikijs/vscode-textmate": "^10.0.2", "@types/hast": "^3.0.4" } }, "sha512-0qWBkM2t/0NXPRcVgtLhtHv6Ak3Q5yI4K/ggMqcgLRKm4+pCs3namgZlhlat/7u2CuqNtlShNs9lENOG6n7UaQ=="],
Expand Down
5 changes: 4 additions & 1 deletion packages/knip/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,5 +128,8 @@
"unresolved",
"unused",
"workspace"
]
],
"optionalDependencies": {
"@actions/core": "^1.11.1"
}
}
32 changes: 32 additions & 0 deletions packages/knip/src/reporters/githubActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import core from '@actions/core';
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webpro Do we want to depend on GitHub Actions as a dependency of Knip? That seems like a lot of overhead for a single log reporter. If not, we could probably easily reverse-engineer the Actions logger. The syntax is pretty simple.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was just happy to reduce deps to this: https://npmgraph.js.org/?q=knip

On the other hand, I don't know what it involves to replicate that logger?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When i did the initial draft of the reporter, i tried to replicate it using console.log and it did not work.

I think its ok marking actions/core as a peer dependency and just document that when using github actions reporter, @actions/core must also be installed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to optionalDependencies, because I personally am not a fan of using peers for things that are, well.. optional. Thoughts?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right, optional is the best case here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I'm exaggerating after trying to cut down on Knip's package size, but looking at their implementation I do have mixed feelings about adding this dependency. An optional dependency is still a dependency.

Even though it's certainly isn't trivial and precision with custom output syntax is required, but essentially we're using three methods and they're all logging to the stdout. Is there any chance you could implement/borrow only the necessary parts?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds good to me. I would be similarly hesitant, which is why I asked. I haven't had a chance to test this against my CI yet (which is why it's still a draft), but I'll try to reimplement it simply to avoid adding the dependency once I do get a chance.

Copy link

@fregante fregante Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optionalDependencies are still installed, even if they don't show up on npmgraph:

Details: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#optionaldependencies

If you don't want it to be a dependency locally you have a few options:

  • make it a plugin (only people who use it install it) — not great
  • offer knip as an action, so it can have its own deps
  • install it on the fly when you detect the GHA environment. This might be troublesome in practice.

Copy link
Contributor

@cylewaitforit cylewaitforit Jun 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the action took just took in a json report like https://github.com/ataylormet/eslint-annotate-action does, then the core of knip itself wouldn’t need the action dependency.

Disregard, I see that this may not even need that level of complexity for basic annotation.

import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from 'src/constants.js';
Copy link

@arthurfiorette arthurfiorette Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from 'src/constants.js';
import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from '../constants.js';

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in my original draft, you cannot use ISSUE_TYPES directly because it has files and not _files.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that the disclosure reporter works around this like so:

knip/packages/knip/src/reporters/disclosure.ts

Line 32 in 5c91dfa

if (reportType === 'files') reportType = '_files';

Should I just copy that pattern?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think so

import type { ReporterOptions } from '../types/issues.js';

export default ({ issues }: ReporterOptions) => {
for (const issueName of ISSUE_TYPES) {
const issue = issues[issueName];

const issueSet =
issue instanceof Set ? Array.from(issue) : Object.values(issue).flatMap(record => Object.values(record));

for (const issueItem of issueSet) {
if (typeof issueItem === 'string') {
core.info(issueItem);
continue;
}
if (issueItem.isFixed || issueItem.severity === 'off') {
continue;
}

const log = issueItem.severity === 'error' ? core.error : core.warning;

log(ISSUE_TYPE_TITLE[issueItem.type], {
file: issueItem.filePath,
startLine: issueItem.line,
endLine: issueItem.line,
endColumn: issueItem.col,
startColumn: issueItem.col,
});
}
}
};
2 changes: 2 additions & 0 deletions packages/knip/src/reporters/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import codeclimate from './codeclimate.js';
import codeowners from './codeowners.js';
import compact from './compact.js';
import disclosure from './disclosure.js';
import githubActions from './githubActions.js';
import json from './json.js';
import markdown from './markdown.js';
import symbols from './symbols.js';
Expand All @@ -14,4 +15,5 @@ export default {
codeclimate,
json,
markdown,
githubActions,
};
16 changes: 16 additions & 0 deletions packages/knip/test/cli/cli-reporter-githubActions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { test } from 'bun:test';
import assert from 'node:assert/strict';
import { resolve } from '../../src/util/path.js';
import { exec } from '../helpers/exec.js';

const cwd = resolve('fixtures/module-resolution-non-std');

test('knip --reporter githubActions (files, unlisted & unresolved)', () => {
Copy link

@arthurfiorette arthurfiorette Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess would be great to use the github actions reporter in ci.yml since this is github :)

assert.equal(
exec('knip --reporter githubActions', { cwd }).stdout,
`${cwd}/src/unused.ts
::error file=${cwd}/src/index.ts::Unlisted dependencies
::error file=${cwd}/src/index.ts::Unlisted dependencies
::error file=${cwd}/src/index.ts,line=8,endLine=8,col=23,endColumn=23::Unresolved imports`
);
});