-
Notifications
You must be signed in to change notification settings - Fork 371
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
Guided Remediation Docs #827
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
+ Coverage 59.78% 59.82% +0.04%
==========================================
Files 136 139 +3
Lines 11268 11466 +198
==========================================
+ Hits 6737 6860 +123
- Misses 4102 4160 +58
- Partials 429 446 +17 ☔ View full report in Codecov by Sentry. |
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.
nice! some initial comments.
docs/guided-remediation.md
Outdated
{: .note } | ||
This feature is experimental and may change in future updates. | ||
|
||
## Basic Usage: Interactive Mode |
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.
Thinking more, we should start with the CLI usage, and then transition to the interactive mode as an alternative for power users.
Starting with a heavyweight interactive mode might scare off readers.
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.
Moved non-interactive to be 'basic usage', with some forward links to the sections describing in-place/relock strategies.
docs/guided-remediation.md
Outdated
|
||
{: .note } | ||
|
||
> The `package-lock.json` file is regenerated by first deleting the existing `package-lock.json` & `node_modules/` directory, then running `npm install --package-lock-only`. To recreate `node_modules/`, you'll need to run `npm ci` separately |
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.
This bit feels a bit confusing to me. The first sentence does not make it clear that node_modules/ will be NOT be recreated along with package-lock.json. We should clarify that.
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.
I've changed it to be more explicit, but I'm not sure if I've made it any less confusing.
docs/guided-remediation.md
Outdated
|
||
A vulnerability is only considered if it satisfies all the conditions set by these flags. | ||
|
||
### Dependency Upgrade Options |
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.
do we still consider to refactor this part of flags to only one flag?
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.
I would like to, but I need to implement it first 🙂
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.
Thanks! Added some comments
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.
LGTM!
docs/guided-remediation.md
Outdated
- Non-registry dependencies (local paths, URLs, Git, etc.) are not evaluated. | ||
- `peerDependencies` are not properly considered during dependency resolution (treated as if using `--legacy-peer-deps`). | ||
- `overrides` are ignored during dependency resolution. | ||
- The `node_modules/` in workspaces are not deleted when relocking, which may impact the resulting dependency graph. |
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.
Also, should we add a section specifically to call out workspaces and how we handle that?
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.
Expanded upon the workspaces in its own sub-sub-sub-heading :)
docs/guided-remediation.md
Outdated
|
||
Guided remediation aims to help developers with fixing with the high number of known vulnerabilities in dependencies typically reported by vulnerability scanners by providing a small number of actionable steps. | ||
|
||
The `osv-scanner fix` subcommand leverages [deps.dev](https://deps.dev) to provide automated and guided remediation of vulnerabilities in your project's dependencies by suggesting upgrades to dependencies. |
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.
This now feels a bit repetitive. How about folding this into the previous paragraph, and moving the deps.dev mention to the resolution point below?
Guided remediation (`osv-scanner fix`) aims to help developers with ....
...
...
- Resolution and analysis of the entire transitive graph (leveraging deps.dev)...
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.
Done
Link: https://michaelkedar.github.io/osv-scanner/experimental/guided-remediation/
Doc page for guided remediation. Will appreciate feedback if things aren't clear or if something's missing.
#352