-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
Knip configuration for keygen #5
Knip configuration for keygen #5
Conversation
Set correct entry points in knip.ts for keygen repository to find all dependencies
Sync with ubiquibot-kernel settings.
Hi @rndquu QA on my fork: https://github.com/gitcoindev/keygen.ubq.fi/actions/runs/8343277802/job/22833177887?pr=2 In case 'Resource not accessible' error shows up, |
Converting into Draft for now. |
… and issues for knip workflow
193a5d7
to
5845df6
Compare
Hi @rndquu I tried to implement the same approach as in ubiquity/ts-template#37 , unfortunately it does not work for knip-reporter as I am wondering if there is any other way, currently experimenting perhaps we could expose and securely use a fine grained PAT token just to allow comments on PRs and run checks permission https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28#create-a-check-run. |
Yes, we could use UbiquiBot Continuous Deploys github app which already has write permissions to PRs (example how to get installation token) |
It's a bit like chicken and egg problem, tried to apply https://github.com/ubiquity/devpool-directory/blob/6c183455f3b78f6e3afe45182345463f01a3dbb3/.github/workflows/sync-issues.yml#L34-L39 but knip-reporter requires
and use yarn install with --mode=skip-build as in https://github.com/ubiquity/devpool-directory/blob/6c183455f3b78f6e3afe45182345463f01a3dbb3/.github/workflows/sync-issues.yml#L32C26-L32C44 If it will just run knip, there should not be hopefully any leaking secrets. I will still experiment on my fork today evening. |
Maybe we can rename the bot to
Consider using footnotes it will make your comment more readable. Example1
Footnotes
|
This approach requires manual approval. It means that a malicious code in any at least medium size PR will eventually be overlooked.
This approach also doesn't work because We could:
|
All right! I finally have a secure solution on my fork that works correctly. Both on pull requests from branches in the same repository and on pull requests from forks. It avoids insecure The algorithm is as follows:
This is quite generic and similar to Clouldflare deployment action, but does not use built artifacts. Knip reporter was not prepared to consume JSON input from an another run and I did not want to pass all files from the pull request. Therefore I forked the knip reporter and implemented this feature as well. I will clean up commits and prepare a pull request to this repository today. After merged and tested here will fix all the other repositories as well. |
Knip workflow uses pull_request. If Knip result fails, Knip-reporter workflow is triggered which updates Knip report directly in the pull request summary and inline comments.
QA with added an unused
Failed Knip workflow triggered Knip reporter https://github.com/gitcoindev/keygen.ubq.fi/actions/runs/8558588746/job/23453470014 and updated findings directly in the pull request.
Failed Knip workflow triggered Knip reporter |
I marked as ready to review and added @gentlementlegen to reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work
is there any QA on positive knip, not failing and running without errors? |
Yes, for example: https://github.com/korrrba/keygen.ubq.fi/pull/15/checks |
Resolves #4