Skip to content

chore: iaw uses org setting insted of feature flag #396

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 23 additions & 0 deletions internal/api/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type ApiClient interface {
GetUserMe() (string, error)
GetSelf() (contract.SelfResponse, error)
GetSastSettings(orgId string) (*sast_contract.SastResponse, error)
GetOrgSettings(orgId string) (*contract.OrgSettingsResponse, error)
}

var _ ApiClient = (*snykApiClient)(nil)
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: use clientGet() instead

if err != nil {
return nil, fmt.Errorf("unable to retrieve org settings: %w", err)
}
//goland:noinspection GoUnhandledErrorResult
defer res.Body.Close()

body, err := io.ReadAll(res.Body)
if err != nil {
return nil, fmt.Errorf("unable to retrieve org settings: %w", err)
}

var response contract.OrgSettingsResponse
if err = json.Unmarshal(body, &response); err != nil {
return nil, fmt.Errorf("unable to retrieve org settings (status: %d): %w", res.StatusCode, err)
}

return &response, err
}
Copy link

Choose a reason for hiding this comment

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

Bug: API Client Fails to Handle Non-OK Status Codes

The GetOrgSettings method fails to validate the HTTP status code before unmarshaling the response. This causes it to attempt parsing API error responses (e.g., 404, 500) as a valid OrgSettingsResponse, resulting in confusing JSON unmarshaling errors instead of clear API-specific errors. Other API client methods, like clientGet(), correctly check for http.StatusOK.

Fix in Cursor Fix in Web

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: I think it's suggesting to use clientGet(), which wraps a.client.Get() with status code checks.


// clientGet performs an HTTP GET request to the Snyk API, handling query parameters,
// API versioning, and basic error checking.
//
Expand Down
16 changes: 16 additions & 0 deletions internal/api/contract/OrgSettingsResponse.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package contract

type OrgIgnoreSettings struct {
ReasonRequired bool `json:"reasonRequired,omitempty"`
AutoApproveIgnores bool `json:"autoApproveIgnores,omitempty"`
ApprovalWorkflowEnabled bool `json:"approvalWorkflowEnabled,omitempty"`
}

type OrgRequestAccessSettings struct {
Enabled bool `json:"enabled,omitempty"`
}

type OrgSettingsResponse struct {
Ignores OrgIgnoreSettings `json:"ignores"`
RequestAccess OrgRequestAccessSettings `json:"requestAccess"`
}
15 changes: 15 additions & 0 deletions internal/mocks/api.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions pkg/local_workflows/ignore_workflow/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/google/uuid"

"github.com/snyk/error-catalog-golang-public/cli"
"github.com/snyk/go-application-framework/internal/api"
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 @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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)
if err != nil {
engine.GetLogger().Err(err).Msg("Failed to access settings.")
return nil, err
}

return settings.Ignores.ApprovalWorkflowEnabled, nil
}
}

func remoteRepoUrlDefaultFunc(existingValue interface{}, config configuration.Configuration) (interface{}, error) {
if existingValue != nil && existingValue != "" {
return existingValue, nil
Expand Down
18 changes: 13 additions & 5 deletions pkg/local_workflows/ignore_workflow/ignore_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,11 @@ import (

"github.com/snyk/code-client-go/sarif"
"github.com/snyk/error-catalog-golang-public/cli"
"github.com/snyk/error-catalog-golang-public/snyk_errors"

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"
"github.com/snyk/go-application-framework/pkg/local_workflows/config_utils"
localworkflows "github.com/snyk/go-application-framework/pkg/local_workflows"
"github.com/snyk/go-application-framework/pkg/local_workflows/local_models"
"github.com/snyk/go-application-framework/pkg/utils/git"
"github.com/snyk/go-application-framework/pkg/workflow"
Expand Down Expand Up @@ -60,6 +60,8 @@ const (

interactiveEnsureVersionControlMessage = "👉🏼 Ensure the code containing the issue is committed and pushed to remote origin, so approvers can review it."
interactiveIgnoreRequestSubmissionMessage = "✅ Your ignore request has been submitted."

ConfigIgnoreApprovalEnabled = "internal_iaw_enabled"
)

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

config_utils.AddFeatureFlagToConfig(engine, configuration.FF_IAW_ENABLED, "ignoreApprovalWorkflow")
engine.GetConfiguration().AddDefaultValue(ConfigIgnoreApprovalEnabled, getOrgIgnoreApprovalEnabled(engine))

return nil
}
Expand All @@ -100,8 +102,14 @@ func ignoreCreateWorkflowEntryPoint(invocationCtx workflow.InvocationContext, _
config := invocationCtx.GetConfiguration()
id := invocationCtx.GetWorkflowIdentifier()

if !config.GetBool(configuration.FF_IAW_ENABLED) {
return nil, cli.NewFeatureUnderDevelopmentError("")
if !config.GetBool(ConfigIgnoreApprovalEnabled) {
return nil, snyk_errors.Error{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change this to an error coming from the Error Catalog before merging.

ID: "SNYK-CLI-0014",
Title: "Organization setting not enabled",
Description: "The feature you are trying to use is not enabled you the current organization. Enable it in the settings or try switching the organization.",
Detail: "",
Level: "fatal",
}
}

interactive := config.GetBool(InteractiveKey)
Expand Down
6 changes: 3 additions & 3 deletions pkg/local_workflows/ignore_workflow/ignore_workflow_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func setupMockIgnoreContext(t *testing.T, payload string, statusCode int) *mocks
config := configuration.New()
config.Set(configuration.API_URL, "https://api.snyk.io")
config.Set(configuration.ORGANIZATION, uuid.New().String())
config.Set(configuration.FF_IAW_ENABLED, true)
config.Set(ConfigIgnoreApprovalEnabled, true)
// setup mocks
ctrl := gomock.NewController(t)
networkAccessMock := mocks.NewMockNetworkAccess(ctrl)
Expand Down Expand Up @@ -306,7 +306,7 @@ func Test_ignoreCreateWorkflowEntryPoint(t *testing.T) {

invocationContext := setupMockIgnoreContext(t, "{}", http.StatusCreated)
config := invocationContext.GetConfiguration()
config.Set(configuration.FF_IAW_ENABLED, false)
config.Set(ConfigIgnoreApprovalEnabled, false)

config.Set(InteractiveKey, false)

Expand All @@ -330,7 +330,7 @@ func setupInteractiveMockContext(t *testing.T, mockApiResponse string, mockApiSt
config := configuration.New()
config.Set(configuration.API_URL, "https://api.snyk.io")
config.Set(configuration.ORGANIZATION, uuid.New().String())
config.Set(configuration.FF_IAW_ENABLED, true)
config.Set(ConfigIgnoreApprovalEnabled, true)
config.Set(InteractiveKey, true) // Always interactive
config.Set(EnrichResponseKey, true)
config.Set(configuration.ORGANIZATION_SLUG, "some-org")
Expand Down