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: support unmanaged snyk security url #2539

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

anthogez
Copy link
Contributor

@anthogez anthogez commented Jan 11, 2022

Co-authored-by: @danlucian lucian.rosu@snyk.io

Allows users with snykUnmanagedVulnDB ff in org level to visualize
https://security.snyk.io/vuln/SNYK-UNMANAGED-CURL-2317584 rather than
nvd url

Depends on snyk/snyk-cpp-plugin#61

with feature flag
Screen Shot 2022-01-11 at 11 53 47

without feature flag
Screen Shot 2022-01-11 at 11 53 03

@anthogez anthogez force-pushed the feat/unmanaged-security-url branch from 7dc957d to 74a9d26 Compare January 11, 2022 10:08
@danlucian danlucian force-pushed the feat/unmanaged-security-url branch 5 times, most recently from 2df80c0 to b7eec17 Compare January 12, 2022 09:25
@anthogez anthogez marked this pull request as ready for review January 12, 2022 09:30
@anthogez anthogez requested review from a team as code owners January 12, 2022 09:30
expect(isUnmanagedEcosystem('docker')).toBeFalsy();
expect(isUnmanagedEcosystem('code')).toBeFalsy();
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@danlucian good work here! What if we go with?

describe('isUnmanagedEcosystem fn', () => {
  it.each`
    actual      | expected
    ${'cpp'}    | ${true}
    ${'docker'} | ${false}
    ${'code'}   | ${false}
  `(
    'should validate that given $actual as input, is considered or not an unmanaged ecosystem',
    ({ actual, expected }) => {
      expect(isUnmanagedEcosystem(actual)).toEqual(expected);
    },
  );
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2022-01-12 at 11 40 38

Copy link
Member

Choose a reason for hiding this comment

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

Looks good, I applied the changes 💪

@danlucian danlucian force-pushed the feat/unmanaged-security-url branch 3 times, most recently from 9f0874d to 31b755c Compare January 12, 2022 09:58
@anthogez
Copy link
Contributor Author

@danlucian let's wait this one to be merged snyk/snyk-cpp-plugin#61
so we can also bump the version of snyk-cpp-plugin here before we merge this pull request :)

@anthogez anthogez force-pushed the feat/unmanaged-security-url branch from 31b755c to dda59c1 Compare January 12, 2022 13:50
Co-authored-by: @danlucian lucian.rosu@snyk.io

Allows users with snykUnmanagedVulnDB ff in org level to visualize
https://security.snyk.io/vuln/SNYK-UNMANAGED-CURL-2317584 rather than
nvd url

Depends on snyk/snyk-cpp-plugin#61

with feature flag
<img width="470" alt="Screen Shot 2022-01-11 at 11 53 47" src="https://user-images.githubusercontent.com/40601533/148922100-369c9d87-73c2-4dcc-881c-4dffac170758.png">

without feature flag
<img width="809" alt="Screen Shot 2022-01-11 at 11 53 03" src="https://user-images.githubusercontent.com/40601533/148922121-d03f6ac3-165c-4539-b8bc-f7ef5ce93ecb.png">
@anthogez anthogez force-pushed the feat/unmanaged-security-url branch from dda59c1 to a3ec49d Compare January 12, 2022 13:53
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

By adding branches for "unmanaged", are we not leaking abstractions? Why isn't unmanaged-specific logic being solved with the plugin itself? Is the plugin API too limited? Should it be improved?

I kind of recall a similar conversation before, but don't remember if a decision was made.

Happy to approve since there are similar existing branches already. Left a comment for some types that need changing.

src/lib/types.ts Show resolved Hide resolved
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.

2 participants