-
Notifications
You must be signed in to change notification settings - Fork 543
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: add org name to snyk code test [NEBULA-195] #2764
Conversation
|
src/lib/plugins/sast/index.ts
Outdated
@@ -45,6 +45,9 @@ export const codePlugin: EcosystemPlugin = { | |||
} | |||
const numOfIssues = sarifTypedResult!.runs?.[0].results?.length || 0; | |||
analytics.add('sast-issues-found', numOfIssues); | |||
if (!options.org && sastSettings.org) { | |||
options.org = sastSettings.org; |
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 not mutate the option.org
object. this might cause some side effects that we don't expect. Instead, we can in every place we check for options.org
use options.org || sastSettings.org
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 agree that changing option.org
is not the best thing. However, to solve this problem we will need the new org in the meta
const in the next line. I didn't want to change how getMeta
is called, so what would be the best solution here? I would have to change the arguments of getMeta
to either include the options.org || sastSettings.org
(or a new const with this result), or just the sastSettings/sastSettings.org
. Do you think this is a good solution, or do I need a new approach here?
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.
@ArturSnyk I think this is not a bad approach. It's basically saying - if no org
defined in options let that org
be the default one. Also, getMeta
is the only consumer of options and all it does is extract the relevant information.
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.
@patricia-v have you looked at how other snyk
commands assign default org
? Like snyk test
. As far as I know, this issue is handled there so maybe we can actually use that solution?
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.
@pkey I have, it is done in a very different way, even in the formatting of the output in the CLI. snyk test
requests from a few APIs that snyk code test
doesn't, and one of those returns the org. Somewhere down the line the response of this request is used as an argument to format the output, including the Organization
.
In this case I think it would be similar to passing the sastSettings
in the arguments of getMeta
.
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.
@patricia-v sounds good then.
[suggestion] let's also add tests for this case? |
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.
Agree with @ArturSnyk's comment, let's have a test to make sure we prevent this issue in the future.
src/lib/plugins/sast/index.ts
Outdated
@@ -45,6 +45,9 @@ export const codePlugin: EcosystemPlugin = { | |||
} | |||
const numOfIssues = sarifTypedResult!.runs?.[0].results?.length || 0; | |||
analytics.add('sast-issues-found', numOfIssues); | |||
if (!options.org && sastSettings.org) { | |||
options.org = sastSettings.org; |
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.
@ArturSnyk I think this is not a bad approach. It's basically saying - if no org
defined in options let that org
be the default one. Also, getMeta
is the only consumer of options and all it does is extract the relevant information.
src/lib/plugins/sast/index.ts
Outdated
@@ -45,6 +45,9 @@ export const codePlugin: EcosystemPlugin = { | |||
} | |||
const numOfIssues = sarifTypedResult!.runs?.[0].results?.length || 0; | |||
analytics.add('sast-issues-found', numOfIssues); | |||
if (!options.org && sastSettings.org) { | |||
options.org = sastSettings.org; |
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.
@patricia-v have you looked at how other snyk
commands assign default org
? Like snyk test
. As far as I know, this issue is handled there so maybe we can actually use that solution?
@patricia-v let me know if you get stuck with the tests we can look together! |
d6b02b5
to
33097ce
Compare
What does this PR do?
When running
snyk code test
, the organization would appear asundefined
.This was solved by passing the
org
from theSastSettings
API endpoint (which is the defaultorg
when not specified by the flag or config) and displaying it in the CLI.Notes for the reviewer
Check the PR on the registry side as well for the API changes.
How should this be manually tested?
Run
snyk code test
in a repository and check the printedOrganization
line. It should not be undefined.Notes:
org
by using the flag--org=org.name
or by setting theorg
in the config by runningsnyk config set org=org.name
snyk config get org
snyk config unset org
More information
Screenshots
Note: The default
org
for this example waspatricia.vale
. The config had no org specified and no flags were used.When running
snyk code test
...Before:
![image](https://user-images.githubusercontent.com/97087926/154458957-198b345a-e8c3-43e1-b7c1-32f4358cb83c.png)
After:
![image](https://user-images.githubusercontent.com/97087926/154458978-8194ad67-71bc-464d-bb00-6fd3487622ad.png)