-
Notifications
You must be signed in to change notification settings - Fork 28
feat: add scan with validation func to Scanner interface #298
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
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
pkg/scan.go
Outdated
| return s.runScan(scanItems, scanConfig, false) | ||
| } | ||
|
|
||
| func (s *scanner) ScanWithValidation(scanItems []ScanItem, scanConfig ScanConfig) (*reporting.Report, error) { |
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.
We can add the validate/withValidation bool variable to the ScanConfig struct and adapt existing functions instead of creating a new function. The idea is that eventually ScanConfig will allow any configuration that the current 2ms cli allows.
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.
sounds good, I will make this changes
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.
Yes, this should follow the Option pattern:
https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md
| close(CvssScoreWithoutValidationChan) | ||
| } | ||
|
|
||
| func ProcessSecretsWithValidation() { |
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.
what is the reason to create this function? can we follow a similar strategy to the preRun function from cmd/main.go? The idea is to eventually refactor to have common logic from the 2ms cli flow instead of duplicated code, so it would be better to not create new logic
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.
because it will require to change ProcessSecrets() signature, I dont know if it will cause breaking changes in other places that importing this repo
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 will it require to change the ProcessSecrets() signature?
pkg/scan_test.go
Outdated
| } | ||
|
|
||
| testScanner := NewScanner() | ||
| actualReport, err := testScanner.ScanWithValidation(scanItems, ScanConfig{}) |
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.
As you probably know, current we have an issue if we try to call the Scan function more than 1 time in a life cycle of our program (it probably has an easy fix, we just didn't have time for it yet). We tested calling this function twice in a row and we receive "close of a closed channel", so just be aware if your use case demands calling it multiple times
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.
Hi it being called once in each execution of a CLI command, so i think it will be ok
| if withValidation { | ||
| go cmd.ProcessSecretsWithValidation() | ||
| go cmd.ProcessSecretsExtras() | ||
| go cmd.ProcessValidationAndScoreWithValidation(engineInstance) | ||
| } else { | ||
| go cmd.ProcessSecrets() | ||
| go cmd.ProcessSecretsExtras() | ||
| go cmd.ProcessScoreWithoutValidation(engineInstance) | ||
| } |
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.
| if withValidation { | |
| go cmd.ProcessSecretsWithValidation() | |
| go cmd.ProcessSecretsExtras() | |
| go cmd.ProcessValidationAndScoreWithValidation(engineInstance) | |
| } else { | |
| go cmd.ProcessSecrets() | |
| go cmd.ProcessSecretsExtras() | |
| go cmd.ProcessScoreWithoutValidation(engineInstance) | |
| } | |
| if withValidation { | |
| go cmd.ProcessSecretsWithValidation() | |
| go cmd.ProcessValidationAndScoreWithValidation(engineInstance) | |
| } else { | |
| go cmd.ProcessSecrets() | |
| go cmd.ProcessScoreWithoutValidation(engineInstance) | |
| } | |
| go cmd.ProcessSecretsExtras() |
| @@ -1,4 +1,4 @@ | |||
| module github.com/checkmarx/2ms | |||
| module github.com/checkmarx/2ms/v3 | |||
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 jumping on the version here? If we don't have a strong reason to do this, we can stay here and not make people update their imports.
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 needed to add to the module name the v3 suffix (this is go conventions in order to be able to import releases with major of 3..)
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 understand that but my question is, why do we need to bump the version? If we are not making a significant change to the project and as far as I understand, we can add the suggested feature without impacting the current use of the 2ms, the version bump is not needed, right?
pkg/scan.go
Outdated
| return s.runScan(scanItems, scanConfig, false) | ||
| } | ||
|
|
||
| func (s *scanner) ScanWithValidation(scanItems []ScanItem, scanConfig ScanConfig) (*reporting.Report, error) { |
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.
Yes, this should follow the Option pattern:
https://github.com/tmrts/go-patterns/blob/master/idiom/functional-options.md
<!-- Thanks for contributing to 2ms by offering a pull request. --> Closes # **Proposed Changes** <!-- Please describe the big picture of your changes here. If it fixes a bug or resolves a feature request, be sure to link to that issue. --> **Checklist** - [x] I covered my changes with tests. - [ ] I Updated the documentation that is affected by my changes: - [ ] Change in the CLI arguments - [ ] Change in the configuration file I submit this contribution under the Apache-2.0 license.








Closes #
Proposed Changes
Checklist
I submit this contribution under the Apache-2.0 license.