-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add logs and add error if serverDetails not provided #189
Conversation
if err != nil { | ||
err = fmt.Errorf("failed to create jas scanner: %s", err.Error()) | ||
return | ||
} else if jasScanner == nil { |
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.
Under which circumstances that weren't exist before the jasScanner can be nil after we tried to create it? If it is not trivial to infer - please add a comment what can result this
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.
look at CreateScanner
. if len(serverDetails.Url) == 0
is true we log the usual warning and return nil scanner, we should not continue after that just like the check was at JasRunner
. we need a way to separate the case that serverDetails is nil (err, should not be nil at this point) or the user did not configure the right things (URL, warning)
if serverDetails == nil || len(serverDetails.Url) == 0 { | ||
log.Warn("To incorporate the ‘Advanced Security’ scans into the audit output make sure platform url is provided and valid (run 'jf c add' prior to 'jf audit' via CLI, or provide JF_URL via Frogbot)") | ||
// Set the analyzer manager executable path. | ||
if scanner.AnalyzerManager.AnalyzerManagerFullPath, err = jas.GetAnalyzerManagerExecutable(); err != nil { |
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.
why are we doing this here? what is changed in the logic that we didn't need it before and now we do?
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.
Nothing has changed. the logic was just moved from one place to another. (Refactoring...)
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 reviewed the changes, there are few things that are unclear to me (see my comments)
Also, about the serverDetails logs, do you think it would suffice to understand the issue with the POC? We know already they dont have the serverDetails or the ServerUrl at this point. In my PR I wanted to understand what they do have (xrayUrl, artifactoryUrl..) or if they lose this data during the process
dev
branch.go vet ./...
.go fmt ./...
.