Skip to content
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 CI #57

Merged
merged 7 commits into from
Jan 6, 2024
Merged

Knip CI #57

merged 7 commits into from
Jan 6, 2024

Conversation

gitcoindev
Copy link

Resolves #897

Quality Assurance:

a) Annotations: https://github.com/gitcoindev/ubiquibot/pull/17/files
b) PR comment: gitcoindev#17 (comment)

Screenshot:

Screenshot from 2023-11-28 12-23-08

run: yarn install

- name: Report knip results to pull request
uses: gitcoindev/[email protected]
Copy link
Owner

Choose a reason for hiding this comment

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

Where can I see this source code?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Owner

@0x4007 0x4007 Dec 6, 2023

Choose a reason for hiding this comment

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

Perhaps its best to place this under Ubiquity? Looks like they updated the code too.

Copy link
Author

Choose a reason for hiding this comment

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

Ok let me check the update.

Copy link
Author

Choose a reason for hiding this comment

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

Let's wait a few days for the upstream pull request to be merged. If it will stall we'll move under Ubiquity, but since the author released knip-reporter with v3 support this week there is a high chance he will pick it up soon.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay keep me posted!

- name: Setup Node
uses: actions/setup-node@v3
with:
node-version: "18.14.1"
Copy link
Owner

Choose a reason for hiding this comment

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

We should use latest LTS

Copy link
Author

Choose a reason for hiding this comment

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

good point, I will check with the latest LTS as well

Copy link
Owner

Choose a reason for hiding this comment

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

Is this finished? Should I merge?

Copy link
Author

Choose a reason for hiding this comment

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

@pavlovcik I will test with latest LTS today and let you know asap.

Copy link
Author

Choose a reason for hiding this comment

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

I rebased to latest knip v3 support in my fork, fixed the tests and opened a PR upstream https://github.com/Codex-/knip-reporter/pull/15 will try to push this, if this is accepted we will be able to use an official release.

Copy link
Author

Choose a reason for hiding this comment

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

My pull request was extended and approved, merged upstream. I will now verify this on my fork and let you know if this works as expected.

Copy link
Author

Choose a reason for hiding this comment

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

I spent quite some time on debugging and asked the author why it does not work as expected on latest upstream release https://github.com/Codex-/knip-reporter/issues/9#issuecomment-1853771495 but does work on my fork let's wait for his response.

Copy link
Author

Choose a reason for hiding this comment

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

@pavlovcik after a few hours I found the culprit and solved the problem.

knip v3 does not work well with typescript < 5 , does not throw errors but its output json does not contain all needed data. I had to update ubiquibot's dev dependencies to typescript ^5.0.4 and jest + ts-jest to v29 and it works like a charm, annotations in action are visible in my qa at https://github.com/gitcoindev/ubiquibot/pull/17/files

It is a bit late for me today, I will check tomorrow if this did not break anything and will add the typescript and jest upgrade as well to refactor/general

Copy link

netlify bot commented Dec 6, 2023

Deploy Preview for ubiquibot-production failed.

Name Link
🔨 Latest commit 2243c38
🔍 Latest deploy log https://app.netlify.com/sites/ubiquibot-production/deploys/657701499664a300095b544f

@0x4007
Copy link
Owner

0x4007 commented Dec 15, 2023

CI failed. What's the status on this?

Copy link

ubiquibot bot commented Dec 15, 2023

# Comment event received without a recognized user command.

Copy link

ubiquibot bot commented Dec 15, 2023

# Comment event received without a recognized user command.

@gitcoindev
Copy link
Author

CI failed. What's the status on this?

Permissions issue, I will try to reproduce on my org repo as on my fork it looks fine.

@gitcoindev
Copy link
Author

CI failed. What's the status on this?

Permissions issue, I will try to reproduce on my org repo as on my fork it looks fine.

@pavlovcik I think I found the root cause.

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#changing-the-permissions-in-a-forked-repository

I suspect that your fork's https://github.com/pavlovcik/ubiquibot settings restrict access for my pull requests to add / modify comments and would have to change 'Workflow permissions' settings

Screenshot from 2023-12-19 22-46-41

https://docs.github.com/en/actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token

I will check if perhaps also permissions can be reduced, currently the yaml workflow file uses permissions: write-all which seems too much.

@0x4007
Copy link
Owner

0x4007 commented Dec 19, 2023

Okay I'll try later today

@gitcoindev
Copy link
Author

Hi @pavlovcik I dug deeper to check why CI fails here. It fails because the pull request is opened from a fork to a fork and not to the original bot repository. This situation is described at https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/#improvements-for-public-repository-forks and https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks .

I was able to reproduce the same permission issue in my personal fork https://github.com/gitcoindev/ubiquibot vs my org fork https://github.com/korrrba/ubiquibot as well. Therefore, once the pull request is merged into the original bot repository, the knip ci workflow will work correctly.

@0x4007 0x4007 merged commit 95bcbdc into 0x4007:refactor/general Jan 6, 2024
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants