feat: use gaf to get sast settings #319
Conversation
acke
force-pushed
the
feat/IDE-724_sast_settings_from-gaf
branch
from
734554d to
f9dea3d
Compare
| client := engine.GetNetworkAccess().GetHttpClient() | ||
| url := config.GetString(configuration.API_URL) | ||
| apiClient := apiClientFactory(url, client) | ||
| orgId := config.GetString(configuration.ORGANIZATION) | ||
| if len(orgId) == 0 { | ||
| return existingValue, nil | ||
| } | ||
| response, err := apiClient.GetSastSettings(orgId) |
There was a problem hiding this comment.
can't we have caching here?
There was a problem hiding this comment.
I thought about this and it might not make sense to have caching for this in GAF and leave it up to the consumer to cache if necessary.
| func getSastSettings(engine workflow.Engine) (*common.SastResponse, error) { | ||
| config := engine.GetConfiguration() |
There was a problem hiding this comment.
I'm not sure if this func is still needed.
Shouldn't the code_workflow get the sastSettings from the config?
There was a problem hiding this comment.
| @@ -98,7 +99,7 @@ func Test_Code_entrypoint(t *testing.T) { | |||
| assert.NoError(t, err) | |||
| assert.NotNil(t, rs) | |||
| assert.Equal(t, expectedData, rs[0].GetPayload().(string)) | |||
| assert.Equal(t, 1, sastSettingsCalled) | |||
| assert.Equal(t, 2, sastSettingsCalled) | |||
There was a problem hiding this comment.
Because now its also called when then default func is setup here: initConfiguration to initiate configuration.SAST_SETTINGS
config.AddDefaultValue(configuration.SAST_SETTINGS, defaultFuncGetSastSettings(engine, config, logger, apiClientFactory))
And it is also called in InitCodeWorkflow to set config for code_workflow.ConfigurationSastEnabled.
engine.GetConfiguration().AddDefaultValue(code_workflow.ConfigurationSastEnabled, getSastEnabled(engine))
Before adding the default func, we only did the second config call in the test case.
| var response common.SastResponse | ||
| var defaultResult common.SastResponse |
There was a problem hiding this comment.
@PeterSchafer fyi we need SastResponse struct to be outside internal pkg to use it in ls.
There was a problem hiding this comment.
Understand! Can we find a more descriptive name than common?
| @@ -44,6 +44,28 @@ func defaultFuncOrganizationSlug(engine workflow.Engine, config configuration.Co | |||
| return callback | |||
| } | |||
|
|
|||
| func defaultFuncGetSastSettings(engine workflow.Engine, config configuration.Configuration, logger *zerolog.Logger, apiClientFactory func(url string, client *http.Client) api.ApiClient) configuration.DefaultValueFunction { | |||
There was a problem hiding this comment.
Issue: Please don't add domain logic into app, which is more or less domain independent. There is already a function registered in the code_workflow, please let's use this one.
There was a problem hiding this comment.
So we want:
- Rewrite the engine.GetConfiguration() to have the sast settings instead of just a ConfigurationSastEnabled configuration.
- Setup the callback in code_workflow that will query the API for sast settings whenever asked for.
- NOT setup a callback for sast settings in app.
There was a problem hiding this comment.
I don't think so, what I wanted to say is, to keep product and domain specific logic close to each other. This means in this case that the default function for SAST specific configuration values should be registered here and implemented somewhere there as well.
One benefit is, that these values only get available if an application registers the code workflow and are not wide spread through the code base.
Changes in Go Application Framework:
defaultFuncGetSastSettingscallback function that executes when the snyk-ls asks for sast settings from the Go Application Framework Configuration./pkg/commonto be used in snyk-ls.SAST_SETTINGS