Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 SSCS as scan type for the triage command (AST-72075) #1022
base: main
Are you sure you want to change the base?
Add SSCS as scan type for the triage command (AST-72075) #1022
Changes from all commits
86a543c
50310bf
1a5ed3a
f3fb8d1
2375f6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
The constant
defaultScsScanOverviewPath
is defined but not used anywhere in the code. Please ensure that it is utilized appropriately or remove it if it's not needed.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.
The file handle
file
is not closed after opening. Consider deferring the close operation immediately after checking the error fromos.Open
to avoid resource leaks.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.
The test function name
TestWriteSingleConfigKeyStringNonExistingFile_CreatingTheFileAndWritesTheKey_Success
is misleading as it implies success, but the test does not assert any conditions to verify the success of the operation. Consider adding assertions to check that the key was written successfully.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.
Instead of using a loop to check all other properties, consider using a more efficient method to compare configurations, such as reflecting the structs or using a dedicated configuration comparison function.
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.
The new condition for
params.ScsType
should be placed after all existing conditions to maintain consistency and readability.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.
The
else if
condition forparams.ScsType
should be consistent with the naming convention used for other scan types. Consider renamingparams.ScsType
toparams.SscsType
to match the pull request title and description indicating the addition of SSCS as a scan type.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.
The function
setDefaultPath
is not defined within the provided diff. Ensure that the function is implemented and correctly handles cases wherepath
is not provided, or is an empty string.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.
The function
setDefaultPath
does not seem to be related to the SSCS triaging capabilities as described in the PR. Please ensure that this function is necessary for the feature being implemented and clarify its purpose in the context of SSCS triaging.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.
The condition
if path != defaultPath
is misleading because the function always returnsdefaultPath
regardless of the inputpath
. Consider returning the actual path that is set in the configuration.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.
The function
setDefaultPath
always returnsdefaultPath
regardless of the inputpath
. This behavior is not intuitive based on the function name and description. Consider returning the actual path set (either the inputpath
ordefaultPath
) or renaming the function to reflect its behavior more accurately.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.
The error from
configuration.GetConfigFilePath()
is logged but not handled. If the config file path cannot be retrieved, the function should not attempt to write to it and should handle the error appropriately.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.
The
viper.Set
call has been removed, which means the new default path is not being set in the configuration. Ensure that the configuration is updated with the new default path if this is the intended behavior.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.
The check
if path != defaultPath
should be followed by a return statement if the condition is false, to avoid unnecessary operations when the path is already set to the default.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.
The error from
configuration.SafeWriteSingleConfigKeyString
is logged but not handled. Consider what should happen if the configuration cannot be updated. Should the function return an error, or should there be a fallback mechanism?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.
The function
configuration.SafeWriteSingleConfigKeyString
is called without handling the case where theconfigFilePath
could not be retrieved due to an error. This could lead to unexpected behavior. Ensure that the function is not called ifconfigFilePath
cannot be determined.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.
Logging the error using
logger.PrintfIfVerbose
might not be sufficient for error handling. Consider propagating the error up to the caller ofsetDefaultPath
so that it can be handled appropriately.