Skip to content
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

Refactor engine code to use specific provider traits in more places #3262

Merged
merged 3 commits into from
May 7, 2024
Merged

Refactor engine code to use specific provider traits in more places #3262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
472d2ef
Refactor engine code to use specific provider traits in more places
dmjb May 7, 2024
a101fb2
fix lint
dmjb May 7, 2024
54c65c6
fix messy rebase
dmjb May 7, 2024
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
6 changes: 5 additions & 1 deletion internal/engine/actions/alert/alert.go
View file
Open in desktop
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we first check if the provider implements github and skip if it doesn't?

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'm not sure I agree with this suggestion:

  1. The code copies the existing behaviour: https://github.com/stacklok/minder/blob/main/internal/engine/actions/alert/security_advisory/security_advisory.go#L161
  2. If other aspects of the configuration are incorrect, we also return an 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.

Note in my next PR, I will change the behaviour to print an explicit error if the provider is not Github

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an explicit "error" that gets the engine to mark something as skipped. Anyway, it's fine to do it in a separate PR.

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
Open in desktop
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
Open in desktop
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
39 changes: 12 additions & 27 deletions internal/engine/actions/remediate/gh_branch_protect/gh_branch_protect_test.go
View file
Open in desktop
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,11 @@ 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/telemetry"
pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1"
provifv1 "github.com/stacklok/minder/pkg/providers/v1"
)
Expand All @@ -49,29 +46,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 clients.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 +138,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 +155,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 +180,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 +219,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 +288,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
Open in desktop
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,
JAORMX marked this conversation as resolved.
Show resolved Hide resolved
) (*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
Loading