Skip to content

fix: respect the reason required org setting for ignore creation #409

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: respect the reason required org setting for ignore creation #409

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 69 additions & 9 deletions pkg/local_workflows/ignore_workflow/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/snyk/error-catalog-golang-public/cli"
"github.com/snyk/go-application-framework/internal/api"
"github.com/snyk/go-application-framework/internal/api/contract"
policyApi "github.com/snyk/go-application-framework/internal/api/policy/2024-10-15"
"github.com/snyk/go-application-framework/pkg/configuration"
"github.com/snyk/go-application-framework/pkg/local_workflows/local_models"
Expand Down Expand Up @@ -40,26 +41,55 @@ func addCreateIgnoreDefaultConfigurationValues(invocationCtx workflow.Invocation

config.AddDefaultValue(ReasonKey, func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
isSet := config.IsSet(ReasonKey)
return defaultFuncWithValidator(existingValue, isSet, isValidReason)
reasonRequired := config.GetBool(ConfigIgnoreReasonRequired)
return defaultFuncWithValidator(existingValue, isSet, getReasonValidator(reasonRequired))
})
}

func getOrgIgnoreSettings(engine workflow.Engine) (*contract.OrgSettingsResponse, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: pass in the necessary parameters only, instead of the entire engine

config := engine.GetConfiguration()
org := config.GetString(configuration.ORGANIZATION)
client := engine.GetNetworkAccess().GetHttpClient()
url := config.GetString(configuration.API_URL)
apiClient := api.NewApi(url, client)

settings, err := apiClient.GetOrgSettings(org)
if err != nil {
engine.GetLogger().Err(err).Msg("Failed to access settings.")
return nil, err
}

engine.GetConfiguration().Set(ConfigIgnoreSettings, settings)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: we already have the config declared, any reason not to use it here?

Suggested change
engine.GetConfiguration().Set(ConfigIgnoreSettings, settings)
config.Set(ConfigIgnoreSettings, settings)

return settings, nil
}

func getOrgIgnoreSettingsConfig(engine workflow.Engine) configuration.DefaultValueFunction {
callback := func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
if existingValue != nil {
return existingValue, nil
}

response, err := getOrgIgnoreSettings(engine)
if err != nil {
engine.GetLogger().Err(err).Msg("Failed to access settings.")
return nil, err
}

return response, nil
}
return callback
}

func getOrgIgnoreApprovalEnabled(engine workflow.Engine) configuration.DefaultValueFunction {
return func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
if existingValue != nil {
return existingValue, nil
}

config := engine.GetConfiguration()
org := config.GetString(configuration.ORGANIZATION)
client := engine.GetNetworkAccess().GetHttpClient()
url := config.GetString(configuration.API_URL)
apiClient := api.NewApi(url, client)

settings, err := apiClient.GetOrgSettings(org)
settings, err := getOrgIgnoreSettings(engine)
if err != nil {
engine.GetLogger().Err(err).Msg("Failed to access settings.")
return nil, err
return false, err
}

if settings.Ignores != nil && settings.Ignores.ApprovalWorkflowEnabled {
Expand All @@ -70,6 +100,26 @@ func getOrgIgnoreApprovalEnabled(engine workflow.Engine) configuration.DefaultVa
}
}

func getOrgIgnoreReasonRequired(engine workflow.Engine) configuration.DefaultValueFunction {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest: getOrgIgnoreReasonRequired, getOrgIgnoreSettingsConfig, getOrgIgnoreApprovalEnabled are all making use of getOrgIgnoreSettings. Is getOrgIgnoreSettings something that can be added to config via config.AddDefaultValue? That way we can take advangate of caching to save mulutiple API calls.

return func(_ configuration.Configuration, existingValue interface{}) (interface{}, error) {
if existingValue != nil {
return existingValue, nil
}

settings, err := getOrgIgnoreSettings(engine)
if err != nil {
engine.GetLogger().Err(err).Msg("Failed to access settings.")
return false, err
}

if settings.Ignores != nil {
return settings.Ignores.ReasonRequired, nil
}

return false, nil
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: API Calls Bypass Cache in Workflow

The getOrgIgnoreSettings function always makes an API call to fetch organization settings without first checking if these settings are already cached in the configuration. This leads to redundant network requests when getOrgIgnoreApprovalEnabled and getOrgIgnoreReasonRequired (or other functions) call it multiple times within a single workflow execution.

Additional Locations (1)

Fix in Cursor Fix in Web


func remoteRepoUrlDefaultFunc(existingValue interface{}, config configuration.Configuration) (interface{}, error) {
if existingValue != nil && existingValue != "" {
return existingValue, nil
Expand Down Expand Up @@ -148,6 +198,16 @@ func isValidFindingsId(input string) error {
return nil
}

func getReasonValidator(reasonRequired bool) func(string) error {
if reasonRequired {
return isValidReason
}

return func(input string) error {
return nil
}
}

func isValidReason(input string) error {
if input == "" {
return cli.NewValidationFailureError("The ignore reason cannot be empty. Provide a justification for ignoring this finding.")
Expand Down
13 changes: 11 additions & 2 deletions pkg/local_workflows/ignore_workflow/ignore_workflow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ const (
interactiveIgnoreRequestSubmissionMessage = "✅ Your ignore request has been submitted."

ConfigIgnoreApprovalEnabled = "internal_iaw_enabled"
ConfigIgnoreReasonRequired = "internal_ignore_reason_required"
ConfigIgnoreSettings = "internal_ignore_settings"
)

var reasonPromptHelpMap = map[string]string{
Expand Down Expand Up @@ -90,7 +92,9 @@ func InitIgnoreWorkflows(engine workflow.Engine) error {
return err
}

engine.GetConfiguration().AddDefaultValue(ConfigIgnoreSettings, getOrgIgnoreSettingsConfig(engine))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggest: looks like we do here, can we then not do a config.get(ConfigIgnoreSettings) instead of the repeated calls to getOrgIgnoreSettings?

engine.GetConfiguration().AddDefaultValue(ConfigIgnoreApprovalEnabled, getOrgIgnoreApprovalEnabled(engine))
engine.GetConfiguration().AddDefaultValue(ConfigIgnoreReasonRequired, getOrgIgnoreReasonRequired(engine))

return nil
}
Expand All @@ -115,6 +119,11 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _
return nil, disabledError
}

reasonRequired, reasonRequiredError := config.GetBoolWithError(ConfigIgnoreReasonRequired)
if reasonRequiredError != nil {
return nil, reasonRequiredError
}

interactive := config.GetBool(InteractiveKey)
addCreateIgnoreDefaultConfigurationValues(invocationCtx)

Expand Down Expand Up @@ -184,7 +193,7 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _
if !ok {
reasonHelp = reasonPromptHelp
}
reason, err = promptIfEmpty(reason, userInterface, reasonHelp, reasonDescription, isValidReason)
reason, err = promptIfEmpty(reason, userInterface, reasonHelp, reasonDescription, getReasonValidator(reasonRequired))
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -213,7 +222,7 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _
return nil, cli.NewEmptyFlagOptionError("The expiration flag is required and cannot be empty. Provide it using the --expiration flag. The date format is YYYY-MM-DD or 'never' for no expiration.")
}

if reason == "" {
if reasonRequired && reason == "" {
return nil, cli.NewEmptyFlagOptionError("The reason flag is required and cannot be empty. Provide it using the --reason flag.")
}

Expand Down
Loading