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

fix: correct remediation broken after last refactoring #507

Merged
merged 1 commit into from
May 10, 2019

Conversation

kyegupov
Copy link
Contributor

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Fixes remediation which got broken in v1.161.2

Remediation relies on annotations added to the test result from the Registry. These annotations are added depending on payload.modules, which is a piggybacking field on a request description object. In the latest refactoring, modules were erroneously (and under inverse conditions) assigned to body.modules, which broke the refactoring workflow.

Apart from the fix, now there are:

  • a test that ensures the annotations are added
  • type-checks for Policy, Body and friends

Where should the reviewer start?

The fix itself is at the bottom of src/lib/snyk-test/run-test.ts

The rest is just tests and typechecks.

How should this be manually tested?

snyk wizard now forks on snyk-fixtures/npm-lockfiles/goof-yarn (e.g. not offering to fix braces which it shouldn't).

Any background context you want to provide?

The bug was introduced in 73e96aa

@kyegupov kyegupov self-assigned this May 10, 2019
@kyegupov kyegupov force-pushed the fix/remediation-after-refactoring branch from f39fe7a to 42c2341 Compare May 10, 2019 17:21
@@ -304,13 +311,6 @@ async function assembleLocalPayloads(root, options): Promise<Payload[]> {
body.depGraph = depGraph;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved the code + flipped the condition

@@ -0,0 +1,172 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is artificial, just to be compatible enough with npm-package fixture for the test to run

@@ -0,0 +1,89 @@
import * as tap from 'tap';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this file more or less copies cli-acceptance (we need a mock Registry), but only runs runTest

@kyegupov kyegupov merged commit a1c3cc6 into master May 10, 2019
@kyegupov kyegupov deleted the fix/remediation-after-refactoring branch May 10, 2019 17:57
@snyksec
Copy link

snyksec commented May 10, 2019

🎉 This PR is included in version 1.163.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants