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

rpc-handler fix knip issues #13

Merged
merged 7 commits into from
May 21, 2024

Conversation

gitcoindev
Copy link
Contributor

Resolves #12

QA: https://github.com/gitcoindev/rpc-handler/actions/runs/9164829604

Knip workflow and all findings should be fixed after merging this PR.

I removed node-fetch from direct dependencies as I found references only in node_modules transitive dependencies.

@gitcoindev gitcoindev requested review from molecula451 and rndquu May 20, 2024 20:59
@gitcoindev gitcoindev self-assigned this May 20, 2024
Copy link
Contributor

Lines Statements Branches Functions
Coverage: NaN%
Unknown% (0/0) Unknown% (0/0) Unknown% (0/0)

JUnit

Tests Skipped Failures Errors Time
0 0 💤 0 ❌ 0 🔥 2.932s ⏱️
Coverage Report (0%)
File% Stmts% Branch% Funcs% LinesUncovered Line #s
All files0000 

@gitcoindev
Copy link
Contributor Author

@rndquu , @molecula451 I assume the failed tests were fixed in #6 by @Keyrxng .

@rndquu
Copy link
Member

rndquu commented May 21, 2024

@gitcoindev

Could you:

  1. Merge the latest development branch and resolve conflicts
  2. Bring back the node-fetch package. It is used by the lib/chainlist github submodule here. Besides I got the build error "node-fetch is missing" without this package to be mentioned explicitly in package.json.

@gitcoindev gitcoindev force-pushed the rpc-handler-fix-knip-issues branch from f54c498 to be6d076 Compare May 21, 2024 14:23
@gitcoindev
Copy link
Contributor Author

@gitcoindev

Could you:

  1. Merge the latest development branch and resolve conflicts
  2. Bring back the node-fetch package. It is used by the lib/chainlist github submodule here. Besides I got the build error "node-fetch is missing" without this package to be mentioned explicitly in package.json.

@rndquu thank you for checking this. I synced my fork to the latest development branch and rebased against it, removing two commits from the pr branch. I force pushed the branch this time to avoid clutter, I would be grateful if you checked again.

@rndquu
Copy link
Member

rndquu commented May 21, 2024

There is too much hurdle with knip. It doesn't work as expected. Check this ci run and these errors:

Unused files (2)
src/services/rpc-service.ts
src/services/storage-service.ts

rpc-service.ts and storage-service.ts are clearly used here and here.

Unused dependencies (2)
axios package.json
node-fetch package.json

axios is used here.

At this point it makes sense to stop using knip and remove it completely (at least from this repo) because maintaining knip's CI workflow gets down to ignoring certain files that are definitely part of the project.

@gitcoindev
Copy link
Contributor Author

@rndquu I have just seen it, will try to fix configuration. Once it is fixed, I do not think much maintenance will be needed.

@gitcoindev
Copy link
Contributor Author

@rndquu I have just seen it, will try to fix configuration. Once it is fixed, I do not think much maintenance will be needed.

I was able to reproduce and fix configuration locally, just pushed an update.

@rndquu
Copy link
Member

rndquu commented May 21, 2024

@gitcoindev Do we need the following steps (one, two, three, four) if Knip-reporter workflow simply downloads and displays the results?

I'm this picky because knip-reporter runs in a privileged context with access to org secrets (because of on: workflow_run) and yarn isntall here could be utilized to exfiltrate secrets should we miss a PR with malicious code in the development branch which is checked out here.

@gitcoindev
Copy link
Contributor Author

@gitcoindev Do we need the following steps (one, two, three, four) if Knip-reporter workflow simply downloads and displays the results?

I'm this picky because knip-reporter runs in a privileged context with access to org secrets (because of on: workflow_run) and yarn isntall here could be utilized to exfiltrate secrets should we miss a PR with malicious code in the development branch which is checked out here.

You are picky but right. I will check this in the next 2 hours. It should be possible to skip those steps.

@gitcoindev
Copy link
Contributor Author

All right! I was able to test this at gitcoindev#5 , will push a follow up commit asap.

@gitcoindev
Copy link
Contributor Author

@rndquu I removed the steps at 759fb4d , please check again.

@gitcoindev
Copy link
Contributor Author

QA CI with steps removed: https://github.com/gitcoindev/rpc-handler/actions/runs/9178339860/job/25238013021

@rndquu rndquu merged commit ff31832 into ubiquity:development May 21, 2024
3 checks passed
@rndquu
Copy link
Member

rndquu commented May 21, 2024

@gitcoindev Works fine, thank you

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.

Create correct Knip workflow and Knip configuration for rpc-handler
3 participants