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

Snyk plugin #156

Merged
merged 2 commits into from
May 14, 2024
Merged

Snyk plugin #156

merged 2 commits into from
May 14, 2024

Conversation

dobarx
Copy link
Contributor

@dobarx dobarx commented Apr 19, 2024

@dobarx dobarx marked this pull request as ready for review April 19, 2024 07:23
@@ -248,6 +249,7 @@ func main() {
splunk.Plugin(version, nil),
stixview.Plugin(version),
nistnvd.Plugin(version, nil),
snyk.Plugin(version, nil),
Copy link
Member

Choose a reason for hiding this comment

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

we really should make this a dynamic discovery (for our plugins) with the ability to extend the list with additions from a flat file (or something) 😄 just a note, not in this pull request

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, but the tests would require a bit of a rewrite. After we started using default values and dataspec the plugins can rely on the default values being present and that required values are non-nil. Manually created cty.Values in tests ignore the default values and required value validation checks.

You can take a look at updated tests for the builtin plugins (in #152), I've implemented a function to construct hcl from cty.Value and re-parse it with default values/required values checked appropriately.

In future I guess that tests for "errors if required value is missing" are not needed, the dataspec validates the data already. And after extracting more validation logic into dataspec (like range checks, non empty strings, #158 ) only relatively few niche validation would be made by the plugin code itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I think I will merge it once I rebase it after your PR #152. Since I see it has some utils for these tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated tests using testtools for those spec values. But this did not help much for snyk plugin. Default values for most of the args are nil. Required checks for args org_id & group_id does not work with dataspec because we require one of these two to be set and there is no such constraint ondataspec.

@dobarx dobarx merged commit fb29a80 into main May 14, 2024
7 checks passed
@dobarx dobarx deleted the snyk_plugin_rebase branch May 14, 2024 04:58
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.

None yet

3 participants