chore: iaw uses org setting insted of feature flag #396
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) ✅ code/snyk check is complete. No issues have been found. (View Details) |
| if !config.GetBool(configuration.FF_IAW_ENABLED) { | ||
| return nil, cli.NewFeatureUnderDevelopmentError("") | ||
| if !config.GetBool(configIgnoreApprovalEnabled) { | ||
| return nil, snyk_errors.Error{ |
There was a problem hiding this comment.
I'll change this to an error coming from the Error Catalog before merging.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| } | ||
|
|
||
| type OrgSettingsResponse struct { | ||
| Ignores OrgIgnoreSettings `json:"ignores"` |
There was a problem hiding this comment.
Why only Ignores? Are there not more fields? If we add them now, we don't have to touch the implementation again, when we need a different org setting :).
There was a problem hiding this comment.
Added the only other field there was.
A question for the audience from my side now:
- Should I be marking the fields as pointers since they are marked as optional, even if all of them are boolean values? I was thinking that if a field is not specified then it should be handled as false, but I'm wondering if that's a good assumption.
There was a problem hiding this comment.
Should I be marking the fields as pointers since they are marked as optional, even if all of them are boolean values? I was thinking that if a field is not specified then it should be handled as false, but I'm wondering if that's a good assumption.
I think so, pointers along with the omitempty tag suffix seem to be the accepted pattern in the codebase.
| @@ -43,6 +44,28 @@ func addCreateIgnoreDefaultConfigurationValues(invocationCtx workflow.Invocation | |||
| }) | |||
| } | |||
|
|
|||
| func getOrgIgnoreApprovalEnabled(engine workflow.Engine) configuration.DefaultValueFunction { | |||
| return func(existingValue interface{}) (interface{}, error) { | |||
| if existingValue != nil { | |||
There was a problem hiding this comment.
this should probably be backed by a cache - after xy time (e.g. 30min), it should not use the previous value. This is important for long-running CLIs, e.g. for Language Server.
There was a problem hiding this comment.
The config uses a cache and how long things are cached is based on the a setting for the configuration. Caching is transparent for the default function and it just needs to handle the business logic itself.
| @@ -247,6 +248,28 @@ func (a *snykApiClient) GetSastSettings(orgId string) (*sast_contract.SastRespon | |||
| return &response, err | |||
| } | |||
|
|
|||
| func (a *snykApiClient) GetOrgSettings(orgId string) (*contract.OrgSettingsResponse, error) { | |||
| endpoint := fmt.Sprintf("%s/v1/org/%s/settings", a.url, url.QueryEscape(orgId)) | |||
| res, err := a.client.Get(endpoint) | |||
There was a problem hiding this comment.
suggestion: use clientGet() instead
Previously, the IAW create workflow was gated behind a feature flag check. For EA, customers should be able to opt in/out of using the ignore approval, so a new org setting was added for them to toggle on/off.
This PR just swaps out the feature flag check with an fetching the org settings and checking whether IAW is enabled for the current org before proceeding with the ignore create workflow.
Ref: CLI-1006