Skip to content

Conversation

@G-Rath
Copy link
Collaborator

@G-Rath G-Rath commented Apr 28, 2025

I believe the main change (for us) is that a native context.Context is now used, with all the "cli" stuff living in cli.Command

Resolves #1773

Copy link
Collaborator

@another-rex another-rex left a comment

Choose a reason for hiding this comment

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

Neat!

@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch from be7b3cc to adfd274 Compare April 29, 2025 00:02
@G-Rath
Copy link
Collaborator Author

G-Rath commented Apr 29, 2025

hmm looks like the tests are flakey, so will need to dig into that

@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch from adfd274 to 4762848 Compare April 29, 2025 01:37
@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch 3 times, most recently from 6f541ab to 9d72251 Compare May 20, 2025 00:14
@G-Rath
Copy link
Collaborator Author

G-Rath commented May 20, 2025

ok so this has been interesting - it looks like the parallel logger is just "missing" so output ends up to the wrong logger, though I don't know why - maybe there's some memory address reuse going on that isn't expected..?

However when attempting to craft a small "debug" command to try and dig into that further, I managed to cause a nil pointer panic in urfave/cli around its flag handling when running the small crafted command at high counts (e.g. -count 10000), which I think I've narrowed down to being related to this code (at the least, if I set HideHelp on my command, the panics seem to stop no matter how high I set the count) which makes some sense because it looks like they're manipulating a global.

There's also another global (disableSliceFlagSeparator) being set that -race reports though as that is just a bool it doesn't seem like it'll be a big issue, but overall combined with what I've seen in the issues it looks like we shouldn't be treating urfave/cli as thread-safe - I don't actually think that is a huge deal for us, and they would hopefully be still open to trying and avoiding globals, but I wanted to write this up anyway so it was somewhere 😅

@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch from 33d3687 to 0405b64 Compare May 20, 2025 20:09
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2025

Codecov Report

Attention: Patch coverage is 81.31868% with 34 lines in your changes missing coverage. Please review.

Project coverage is 65.39%. Comparing base (2b782f0) to head (9bc319a).
Report is 59 commits behind head on main.

Files with missing lines Patch % Lines
cmd/osv-scanner/fix/command.go 60.52% 7 Missing and 8 partials ⚠️
cmd/osv-reporter/main.go 0.00% 10 Missing ⚠️
cmd/osv-scanner/scan/source/command.go 95.45% 2 Missing and 1 partial ⚠️
cmd/osv-scanner/internal/helper/helper.go 88.88% 1 Missing and 1 partial ⚠️
cmd/osv-scanner/update/command.go 77.77% 1 Missing and 1 partial ⚠️
cmd/osv-scanner/internal/testcmd/run.go 0.00% 0 Missing and 1 partial ⚠️
cmd/osv-scanner/scan/image/command.go 95.65% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1844      +/-   ##
==========================================
+ Coverage   65.28%   65.39%   +0.11%     
==========================================
  Files         164      164              
  Lines       15868    15926      +58     
==========================================
+ Hits        10359    10415      +56     
- Misses       4845     4847       +2     
  Partials      664      664              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch from 0405b64 to 3ede6b1 Compare May 27, 2025 03:51
@G-Rath G-Rath force-pushed the cmd/update-to-v3 branch from b926016 to 9bc319a Compare May 28, 2025 00:46
@another-rex
Copy link
Collaborator

Tested on a few different machines, seem to work in all the places, so going to merge this in.

@another-rex another-rex merged commit 073ebc6 into google:main May 28, 2025
15 checks passed
@another-rex another-rex deleted the cmd/update-to-v3 branch May 28, 2025 03:30
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.

Update/Migrate to cli/v3

4 participants