Skip to content

Commit

Permalink
Refactor engine code to use specific provider traits in more places
Browse files Browse the repository at this point in the history 
Relates to #2845

When evaluating a policy, we instantiate a ProviderBuilder and then pass
it around to various parts of the code. Once the specific trait is
known, we get the specific trait we need from the ProviderBuilder.

While replacing the ProviderBuilder, I notice that there are many parts
of the code where we continue to pass around the ProviderBuilder even
though we know the specific provider trait which will be needed.
I changed various code to explicitly require the provider trait it needs
instead of taking the ProviderBuilder and getting an instance of the
desired type. As a result of this change, test setup is simplified, and
this will make my PR to replace the ProviderBuilder smaller.
  • Loading branch information
@dmjb
dmjb committed May 7, 2024
1 parent 9d5cd7e commit c239427
Show file tree
Hide file tree
Showing 27 changed files with 211 additions and 600 deletions.
6 changes: 5 additions & 1 deletion internal/engine/actions/alert/alert.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,11 @@ func NewRuleAlert(rt *pb.RuleType, pbuild *providers.ProviderBuilder) (engif.Act
if alertCfg.GetSecurityAdvisory() == nil {
return nil, fmt.Errorf("alert engine missing security-advisory configuration")
}
return security_advisory.NewSecurityAdvisoryAlert(ActionType, rt.GetSeverity(), alertCfg.GetSecurityAdvisory(), pbuild)
client, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("could not instantiate provider: %w", err)
}
return security_advisory.NewSecurityAdvisoryAlert(ActionType, rt.GetSeverity(), alertCfg.GetSecurityAdvisory(), client)
}

return nil, fmt.Errorf("unknown alert type: %s", alertCfg.GetType())
Expand Down
9 changes: 2 additions & 7 deletions internal/engine/actions/alert/security_advisory/security_advisory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (

enginerr "github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/util"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/minder/pkg/providers/v1"
Expand Down Expand Up @@ -135,7 +134,7 @@ func NewSecurityAdvisoryAlert(
actionType interfaces.ActionType,
sev *pb.Severity,
saCfg *pb.RuleType_Definition_Alert_AlertTypeSA,
pbuild *providers.ProviderBuilder,
cli provifv1.GitHub,
) (*Alert, error) {
if actionType == "" {
return nil, fmt.Errorf("action type cannot be empty")
Expand All @@ -155,11 +154,7 @@ func NewSecurityAdvisoryAlert(
if err != nil {
return nil, fmt.Errorf("cannot parse description template: %w", err)
}
// Get the GitHub client
cli, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("cannot get http client: %w", err)
}

// Create the alert action
return &Alert{
actionType: actionType,
Expand Down
7 changes: 1 addition & 6 deletions internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import (

engerrors "github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/providers"
mindergh "github.com/stacklok/minder/internal/providers/github"
"github.com/stacklok/minder/internal/util"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
Expand All @@ -55,7 +54,7 @@ type GhBranchProtectRemediator struct {
func NewGhBranchProtectRemediator(
actionType interfaces.ActionType,
ghp *pb.RuleType_Definition_Remediate_GhBranchProtectionType,
pbuild *providers.ProviderBuilder,
cli provifv1.GitHub,
) (*GhBranchProtectRemediator, error) {
if actionType == "" {
return nil, fmt.Errorf("action type cannot be empty")
Expand All @@ -66,10 +65,6 @@ func NewGhBranchProtectRemediator(
return nil, fmt.Errorf("cannot parse patch template: %w", err)
}

cli, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("cannot get http client: %w", err)
}
return &GhBranchProtectRemediator{
actionType: actionType,
cli: cli,
Expand Down
40 changes: 13 additions & 27 deletions internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ package gh_branch_protect

import (
"context"
"database/sql"
"encoding/json"
"fmt"
"strings"
"testing"
Expand All @@ -29,12 +27,12 @@ import (
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/known/structpb"

serverconfig "github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/providers/credentials"
"github.com/stacklok/minder/internal/providers/github/clients"
mock_ghclient "github.com/stacklok/minder/internal/providers/github/mock"
"github.com/stacklok/minder/internal/providers/github/oauth"
"github.com/stacklok/minder/internal/providers/telemetry"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/minder/pkg/providers/v1"
)
Expand All @@ -49,29 +47,19 @@ const (

var TestActionTypeValid interfaces.ActionType = "remediate-test"

func testGithubProviderBuilder(baseURL string) *providers.ProviderBuilder {
func testGithubProvider(baseURL string) (provifv1.GitHub, error) {
if !strings.HasSuffix(baseURL, "/") {
baseURL = baseURL + "/"
}

definitionJSON := `{
"github": {
"endpoint": "` + baseURL + `"
}
}`

return providers.NewProviderBuilder(
&db.Provider{
Name: "github",
Version: provifv1.V1,
Implements: []db.ProviderType{db.ProviderTypeGithub, db.ProviderTypeRest},
Definition: json.RawMessage(definitionJSON),
return oauth.NewRestClient(
&pb.GitHubProviderConfig{
Endpoint: baseURL,
},
sql.NullString{},
false,
nil,
credentials.NewGitHubTokenCredential("token"),
&serverconfig.ProviderConfig{},
nil, // this is unused here
clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),
"",
)
}

Expand Down Expand Up @@ -151,7 +139,6 @@ func TestBranchProtectionRemediate(t *testing.T) {

type newBranchProtectionRemediateArgs struct {
ghp *pb.RuleType_Definition_Remediate_GhBranchProtectionType
pbuild *providers.ProviderBuilder
actionType interfaces.ActionType
}

Expand All @@ -169,7 +156,6 @@ func TestBranchProtectionRemediate(t *testing.T) {
ghp: &pb.RuleType_Definition_Remediate_GhBranchProtectionType{
Patch: reviewCountPatch,
},
pbuild: testGithubProviderBuilder(ghApiUrl),
actionType: "",
},
mockSetup: func(_ *mock_ghclient.MockGitHub) {
Expand All @@ -195,7 +181,6 @@ func TestBranchProtectionRemediate(t *testing.T) {
ghp: &pb.RuleType_Definition_Remediate_GhBranchProtectionType{
Patch: reviewCountPatch,
},
pbuild: testGithubProviderBuilder(ghApiUrl),
actionType: TestActionTypeValid,
},
remArgs: &remediateArgs{
Expand Down Expand Up @@ -235,7 +220,6 @@ func TestBranchProtectionRemediate(t *testing.T) {
ghp: &pb.RuleType_Definition_Remediate_GhBranchProtectionType{
Patch: reviewCountPatch,
},
pbuild: testGithubProviderBuilder(ghApiUrl),
actionType: TestActionTypeValid,
},
remArgs: &remediateArgs{
Expand Down Expand Up @@ -305,7 +289,9 @@ func TestBranchProtectionRemediate(t *testing.T) {

mockClient := mock_ghclient.NewMockGitHub(ctrl)

engine, err := NewGhBranchProtectRemediator(tt.newRemArgs.actionType, tt.newRemArgs.ghp, tt.newRemArgs.pbuild)
prov, err := testGithubProvider(ghApiUrl)
require.NoError(t, err)
engine, err := NewGhBranchProtectRemediator(tt.newRemArgs.actionType, tt.newRemArgs.ghp, prov)
if tt.wantInitErr {
require.Error(t, err, "expected error")
return
Expand Down
8 changes: 1 addition & 7 deletions internal/engine/actions/remediate/pull_request/pull_request.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import (
"github.com/stacklok/minder/internal/db"
enginerr "github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/util"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/minder/pkg/providers/v1"
Expand Down Expand Up @@ -91,7 +90,7 @@ type paramsPR struct {
func NewPullRequestRemediate(
actionType interfaces.ActionType,
prCfg *pb.RuleType_Definition_Remediate_PullRequestRemediation,
pbuild *providers.ProviderBuilder,
ghCli provifv1.GitHub,
) (*Remediator, error) {
err := prCfg.Validate()
if err != nil {
Expand All @@ -108,11 +107,6 @@ func NewPullRequestRemediate(
return nil, fmt.Errorf("cannot parse body template: %w", err)
}

ghCli, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("failed to get github client: %w", err)
}

modRegistry := newModificationRegistry()
modRegistry.registerBuiltIn()

Expand Down
45 changes: 13 additions & 32 deletions internal/engine/actions/remediate/pull_request/pull_request_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package pull_request
import (
"bytes"
"context"
"database/sql"
"encoding/json"
"fmt"
"io"
Expand All @@ -39,13 +38,13 @@ import (
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/known/structpb"

serverconfig "github.com/stacklok/minder/internal/config/server"
"github.com/stacklok/minder/internal/db"
"github.com/stacklok/minder/internal/engine/errors"
"github.com/stacklok/minder/internal/engine/interfaces"
"github.com/stacklok/minder/internal/providers"
"github.com/stacklok/minder/internal/providers/credentials"
"github.com/stacklok/minder/internal/providers/github/clients"
mock_ghclient "github.com/stacklok/minder/internal/providers/github/mock"
"github.com/stacklok/minder/internal/providers/github/oauth"
"github.com/stacklok/minder/internal/providers/telemetry"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/minder/pkg/providers/v1"
)
Expand Down Expand Up @@ -89,27 +88,15 @@ jobs:

var TestActionTypeValid interfaces.ActionType = "remediate-test"

func testGithubProviderBuilder() *providers.ProviderBuilder {
baseURL := ghApiUrl + "/"

definitionJSON := `{
"github": {
"endpoint": "` + baseURL + `"
}
}`

return providers.NewProviderBuilder(
&db.Provider{
Name: "github",
Version: provifv1.V1,
Implements: []db.ProviderType{db.ProviderTypeGithub, db.ProviderTypeRest, db.ProviderTypeGit},
Definition: json.RawMessage(definitionJSON),
func testGithubProvider() (provifv1.GitHub, error) {
return oauth.NewRestClient(
&pb.GitHubProviderConfig{
Endpoint: ghApiUrl + "/",
},
sql.NullString{},
false,
nil,
credentials.NewGitHubTokenCredential("token"),
&serverconfig.ProviderConfig{},
nil, // this is unused here
clients.NewGitHubClientFactory(telemetry.NewNoopMetrics()),
"",
)
}

Expand Down Expand Up @@ -369,7 +356,6 @@ func TestPullRequestRemediate(t *testing.T) {

type newPullRequestRemediateArgs struct {
prRem *pb.RuleType_Definition_Remediate_PullRequestRemediation
pbuild *providers.ProviderBuilder
actionType interfaces.ActionType
}

Expand All @@ -387,7 +373,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "open a PR",
newRemArgs: &newPullRequestRemediateArgs{
prRem: dependabotPrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand All @@ -410,7 +395,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "fail to open a PR",
newRemArgs: &newPullRequestRemediateArgs{
prRem: dependabotPrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand All @@ -433,7 +417,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "update an existing PR branch with a force-push",
newRemArgs: &newPullRequestRemediateArgs{
prRem: dependabotPrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand All @@ -456,7 +439,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "A PR already exists, use it and don't open a new one",
newRemArgs: &newPullRequestRemediateArgs{
prRem: dependabotPrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand Down Expand Up @@ -518,7 +500,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "resolve tags using frizbee",
newRemArgs: &newPullRequestRemediateArgs{
prRem: frizbeePrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand Down Expand Up @@ -546,7 +527,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "resolve tags using frizbee with excludes",
newRemArgs: &newPullRequestRemediateArgs{
prRem: frizbeePrRemWithExcludes([]string{"actions/setup-go@v5"}),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgs(),
Expand All @@ -572,7 +552,6 @@ func TestPullRequestRemediate(t *testing.T) {
name: "resolve tags using frizbee with excludes from rule",
newRemArgs: &newPullRequestRemediateArgs{
prRem: frizbeePrRem(),
pbuild: testGithubProviderBuilder(),
actionType: TestActionTypeValid,
},
remArgs: createTestRemArgsWithExcludes(),
Expand Down Expand Up @@ -610,7 +589,9 @@ func TestPullRequestRemediate(t *testing.T) {

mockClient := mock_ghclient.NewMockGitHub(ctrl)

engine, err := NewPullRequestRemediate(tt.newRemArgs.actionType, tt.newRemArgs.prRem, tt.newRemArgs.pbuild)
provider, err := testGithubProvider()
require.NoError(t, err)
engine, err := NewPullRequestRemediate(tt.newRemArgs.actionType, tt.newRemArgs.prRem, provider)
if tt.wantInitErr {
require.Error(t, err, "expected error")
return
Expand Down
18 changes: 15 additions & 3 deletions internal/engine/actions/remediate/remediate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,35 @@ func NewRuleRemediator(rt *pb.RuleType, pbuild *providers.ProviderBuilder) (engi
// nolint:revive // let's keep the switch here, it would be nicer to extend a switch in the future
switch rem.GetType() {
case rest.RemediateType:
client, err := pbuild.GetHTTP()
if err != nil {
return nil, fmt.Errorf("could not instantiate provider: %w", err)
}
if rem.GetRest() == nil {
return nil, fmt.Errorf("remediations engine missing rest configuration")
}
return rest.NewRestRemediate(ActionType, rem.GetRest(), pbuild)
return rest.NewRestRemediate(ActionType, rem.GetRest(), client)

case gh_branch_protect.RemediateType:
client, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("could not instantiate provider: %w", err)
}
if rem.GetGhBranchProtection() == nil {
return nil, fmt.Errorf("remediations engine missing gh_branch_protection configuration")
}
return gh_branch_protect.NewGhBranchProtectRemediator(ActionType, rem.GetGhBranchProtection(), pbuild)
return gh_branch_protect.NewGhBranchProtectRemediator(ActionType, rem.GetGhBranchProtection(), client)

case pull_request.RemediateType:
client, err := pbuild.GetGitHub()
if err != nil {
return nil, fmt.Errorf("could not instantiate provider: %w", err)
}
if rem.GetPullRequest() == nil {
return nil, fmt.Errorf("remediations engine missing pull request configuration")
}

return pull_request.NewPullRequestRemediate(ActionType, rem.GetPullRequest(), pbuild)
return pull_request.NewPullRequestRemediate(ActionType, rem.GetPullRequest(), client)
}

return nil, fmt.Errorf("unknown remediation type: %s", rem.GetType())
Expand Down
Loading

0 comments on commit c239427

Please sign in to comment.