ASA 8404 (#158) #159
ASA 8404 (#158) #159
Conversation
* include SCA implementation * copyright changes
| @@ -27,12 +27,13 @@ public interface CoreConstants { | |||
| String SCANNER_TYPE = "type"; //$NON-NLS-1$ | |||
| String STATUS = "Status"; //$NON-NLS-1$ | |||
| String TARGET = "target"; //$NON-NLS-1$ | |||
| String OPEN_SOURCE_ONLY = "openSourceOnly"; //$NON-NLS-1$ | |||
| String INCLUDE_SCA = "includeSCA"; //$NON-NLS-1$ | |||
There was a problem hiding this comment.
The OPEN_SOURCE_ONLY constant is still used in various places within the code, so we should not be removing it. It could also be used by different plugins, so we should keep that constant in place.
| String VERSION_NUMBER = "VersionNumber"; //$NON-NLS-1$ | ||
| String USER_MESSAGE = "UserMessage"; //$NON-NLS-1$ | ||
| String IS_VALID = "IsValid"; //$NON-NLS-1$ | ||
| String SOURCE_CODE_ONLY = "sourceCodeOnly"; //$NON-NLS-1$ | ||
| String SOFTWARE_COMPOSITION_ANALYZER = "Software Composition Analyzer"; //$NON-NLS-1$ | ||
| String STATIC_ANALYZER = "Static Analyzer"; //$NON-NLS-1$ |
There was a problem hiding this comment.
Both SOFTWARE_COMPOSITION_ANALYZER and STATIC_ANALYZER are already defined in the ScannerConstants interface, so we shouldn't duplicate those constants here.
There was a problem hiding this comment.
As we have STATIC_ANALYZER constant in the SASTConstant interface, using the same in SAClient class.
There was a problem hiding this comment.
Can you remove the constant here since it's a duplicate?
| @@ -33,7 +33,11 @@ public ASoCScan(Map<String, String> properties, IScanServiceProvider provider) { | |||
| } | |||
|
|
|||
| public ASoCScan(Map<String, String> properties, IProgress progress, IScanServiceProvider provider) { | |||
| m_target = properties.remove(CoreConstants.TARGET); | |||
| if(properties.containsKey(CoreConstants.INCLUDE_SCA)) { | |||
There was a problem hiding this comment.
What's the reason for keeping the TARGET property when the properties contains INCLUDE_SCA? We have a handle to that value through m_target.
There was a problem hiding this comment.
If 2 scans are executing then this method is being called twice, so during the 2nd run it will throw me error as we had removed the TARGET parameter in the 1st run.
There was a problem hiding this comment.
The behavior you're describing is specific to the Jenkins plugin that uses this SDK. A check for a constant called INCLUDE_SCA doesn't make sense here from the SDK perspective.
There was a problem hiding this comment.
Sure, will handle the same in plugin code.
| @@ -326,16 +326,17 @@ private List<String> getClientArgs(Map<String, String> properties) { | |||
| if(properties.containsKey(VERBOSE)) { | |||
| args.add(OPT_VERBOSE); | |||
| } | |||
| if(properties.containsKey(THIRD_PARTY) || System.getProperty(THIRD_PARTY) != null) { | |||
| if(properties.containsKey(THIRD_PARTY) || System.getProperty(THIRD_PARTY) != null) | |||
There was a problem hiding this comment.
For consistency, we should keep the { } brackets rather than having some conditionals use them and others not.
| if(properties.containsKey(SCAN_SPEED)) { | ||
| if (!properties.containsKey(CoreConstants.INCLUDE_SCA) && properties.get(CoreConstants.SCANNER_TYPE).equals(CoreConstants.STATIC_ANALYZER)) | ||
| args.add(OPT_STATIC_ANALYSIS_ONLY); | ||
| if (!properties.containsKey(CoreConstants.INCLUDE_SCA) && properties.get(CoreConstants.SCANNER_TYPE).equals(CoreConstants.SOFTWARE_COMPOSITION_ANALYZER)) |
There was a problem hiding this comment.
Is the first "if" check necessary here? !properties.containsKey(CoreConstants.INCLUDE_SCA)
If the scanner type is SCA, it shouldn't matter if that property is set or not.
There was a problem hiding this comment.
Okay, I can skip "includeSCA" check for the SCA scan as IRX generation will only take place if ApplicationFileId is not present in properties.
| @@ -34,8 +34,12 @@ public void run() throws ScannerException, InvalidTargetException { | |||
| throw new InvalidTargetException(Messages.getMessage(TARGET_INVALID, target)); | |||
|
|
|||
| try { | |||
| generateIR(); | |||
| analyzeIR(); | |||
| if(getProperties().containsKey(CoreConstants.INCLUDE_SCA) && getProperties().containsKey("ApplicationFileId")) { | |||
There was a problem hiding this comment.
Since this is the SCAScan class, is there a reason to check if INCLUDE_SCA is set? It seems like that would be implied by the scan type.
There was a problem hiding this comment.
For the implementation of include SCA, I am changing the SCANNER_TYPE parameter in the properties to SCA after the execution of 1st scan. This class will be called during the 2nd scan & with the INCLUDE_SCA check i am skipping the irx generation for the SCA execution as we have to utilize the same generated irx of the 1st scan.
There was a problem hiding this comment.
Similar to the other comment, what you're describing is specific to the Jenkins plugin that uses the SDK. This behavior should be handled in that plugin, as opposed to here.
There was a problem hiding this comment.
Okay, I will only check for the "ApplicationFileId" paramter, if it is there we would skip the IRX generation step for SCA scan.
* include SCA implementation * review changes
https://jira02.hclpnp.com/browse/ASA-8404