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

feat: sequential fix #2252

Merged
merged 7 commits into from
Sep 15, 2021
Merged

feat: sequential fix #2252

merged 7 commits into from
Sep 15, 2021

Conversation

lili2311
Copy link
Contributor

@lili2311 lili2311 commented Sep 13, 2021

What does this PR do?

Support optionally upgrading 1 package at a time, this
is much slower but can provide better success in upgrading
vulnerable package versions

Any background context you want to provide?

Some combination of package upgraded might not be compatible, allow upgrading 1 at a time to increase the % of fixed issues. This is much slower 🐌

snyk fix --sequential will trigger the behavior to try applying package upgrades via poetry and pipenv 1 by 1.

Screenshots

Can't see a difference in the screenshot unless one of the upgrades fails. See the added test to show what to expect.

Actual project run
CleanShot 2021-09-14 at 13 32 06@2x

From a test that crafts the right scenario, when running with --sequential-fix the whole fix does not fail.
CleanShot 2021-09-14 at 13 34 45@2x

@lili2311 lili2311 self-assigned this Sep 13, 2021
@lili2311 lili2311 changed the title Feat/sequential fix feat: sequential fix Sep 14, 2021
@lili2311 lili2311 force-pushed the feat/sequential-fix branch 4 times, most recently from d082aca to 31d50d6 Compare September 14, 2021 12:07
@github-actions
Copy link
Contributor

github-actions bot commented Sep 14, 2021

Warnings
⚠️ You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?
⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones.
A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax.
Files found:

  • src/cli/args.ts

Generated by 🚫 dangerJS against a5a618c

Support optionally upgrading 1 package at a time, this
is much slower but can provide better success in upgrading
vulnerable package versions
Move throwable code into try/catch to make sure we continue fixing + spinner
is not stuck
Update a test to show that if some dependencies fail under --sequentially-fix
we can still update other dependencies.
const changes: FixChangesSummary[] = [];
try {
const { upgrades } = await generateUpgrades(entity);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

move back into try/catch to not prevent further fixes & to let the spinner finish

@lili2311 lili2311 force-pushed the feat/sequential-fix branch 5 times, most recently from c85ba8e to 6cd1266 Compare September 14, 2021 13:48
@lili2311 lili2311 marked this pull request as ready for review September 14, 2021 15:07
@lili2311 lili2311 requested review from a team as code owners September 14, 2021 15:07
if asked fix a package 1 by 1, this is much slower
but can increase success % when some packages might be incompatible
Copy link
Contributor

@jan-stehlik jan-stehlik left a comment

Choose a reason for hiding this comment

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

LGTM, nice one @lili2311 🙌

@lili2311 lili2311 merged commit b1313ed into master Sep 15, 2021
@lili2311 lili2311 deleted the feat/sequential-fix branch September 15, 2021 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants