Remove PacOpts from IsAllowed

We already have it instantiated on the run object via SetClient, no need to pass it again. Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>
openshift-pipelines · Nov 13, 2023 · 0fc3592 · 0fc3592
1 parent 90ce7a4
commit 0fc3592
Show file tree

Hide file tree

Showing 14 changed files with 34 additions and 32 deletions.
diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go
@@ -116,7 +116,7 @@ is that what you want? make sure you use -n when generating the secret, eg: echo
 
 	// Check if the submitter is allowed to run this.
 	if p.event.TriggerTarget != "push" {
-		allowed, err := p.vcx.IsAllowed(ctx, p.event, p.run.Info.Pac)
+		allowed, err := p.vcx.IsAllowed(ctx, p.event)
 		if err != nil {
 			return repo, err
 		}

diff --git a/pkg/provider/bitbucketcloud/acl.go b/pkg/provider/bitbucketcloud/acl.go
@@ -11,7 +11,7 @@ import (
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud/types"
 )
 
-func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, _ *info.PacOpts) (bool, error) {
+func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	// Check first if the user is in the owner file or part of the workspace
 	allowed, err := v.checkMember(ctx, event)
 	if err != nil {

diff --git a/pkg/provider/bitbucketcloud/acl_test.go b/pkg/provider/bitbucketcloud/acl_test.go
@@ -180,10 +180,7 @@ func TestIsAllowed(t *testing.T) {
 			bbcloudtest.MuxFiles(t, mux, tt.event, tt.fields.filescontents, "")
 
 			v := &Provider{Client: bbclient}
-
-			pacopts := info.PacOpts{}
-
-			got, err := v.IsAllowed(ctx, tt.event, &pacopts)
+			got, err := v.IsAllowed(ctx, tt.event)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("Provider.IsAllowed() error = %v, wantErr %v", err, tt.wantErr)
 				return

diff --git a/pkg/provider/bitbucketserver/acl.go b/pkg/provider/bitbucketserver/acl.go
@@ -14,7 +14,7 @@ import (
 
 type activitiesTypes struct{ Values []*bbv1.Activity }
 
-func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, _ *info.PacOpts) (bool, error) {
+func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	allowed, err := v.checkMemberShip(ctx, event)
 	if err != nil {
 		return false, err

diff --git a/pkg/provider/bitbucketserver/acl_test.go b/pkg/provider/bitbucketserver/acl_test.go
@@ -217,9 +217,7 @@ func TestIsAllowed(t *testing.T) {
 				projectKey:                tt.event.Organization,
 			}
 
-			pacopts := info.PacOpts{}
-
-			got, err := v.IsAllowed(ctx, tt.event, &pacopts)
+			got, err := v.IsAllowed(ctx, tt.event)
 			if tt.wantErrSubstr != "" {
 				assert.ErrorContains(t, err, tt.wantErrSubstr)
 				return

diff --git a/pkg/provider/gitea/acl.go b/pkg/provider/gitea/acl.go
@@ -44,7 +44,7 @@ func (v *Provider) CheckPolicyAllowing(_ context.Context, event *info.Event, all
 	return false, fmt.Sprintf("user: %s is not a member of any of the allowed teams: %v", event.Sender, allowedTeams)
 }
 
-func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, pac *info.PacOpts) (bool, error) {
+func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	aclPolicy := policy.Policy{
 		Repository:   v.repo,
 		EventEmitter: v.eventEmitter,
@@ -74,7 +74,7 @@ func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, pac *info.P
 	}
 
 	// Try to parse the comment from an owner who has issues a /ok-to-test
-	ownerAllowed, err := v.aclAllowedOkToTestFromAnOwner(ctx, event, pac)
+	ownerAllowed, err := v.aclAllowedOkToTestFromAnOwner(ctx, event)
 	if err != nil {
 		return false, err
 	}
@@ -95,7 +95,7 @@ func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, pac *info.P
 // if there is a /ok-to-test in there running an aclCheck again on the comment
 // Sender if she is an OWNER and then allow it to run CI.
 // TODO: pull out the github logic from there in an agnostic way.
-func (v *Provider) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *info.Event, pac *info.PacOpts) (bool, error) {
+func (v *Provider) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *info.Event) (bool, error) {
 	revent := info.NewEvent()
 	event.DeepCopyInto(revent)
 	revent.EventType = ""
@@ -108,14 +108,14 @@ func (v *Provider) aclAllowedOkToTestFromAnOwner(ctx context.Context, event *inf
 	case *giteaStructs.IssueCommentPayload:
 		// if we don't need to check old comments, then on issue comment we
 		// need to check if comment have /ok-to-test and is from allowed user
-		if !pac.RememberOKToTest {
+		if !v.run.Info.Pac.RememberOKToTest {
 			return v.aclAllowedOkToTestCurrentComment(ctx, revent, event.Comment.ID)
 		}
 		revent.URL = event.Issue.URL
 	case *giteaStructs.PullRequestPayload:
 		// if we don't need to check old comments, then on push event we don't need
 		// to check anything for the non-allowed user
-		if !pac.RememberOKToTest {
+		if !v.run.Info.Pac.RememberOKToTest {
 			return false, nil
 		}
 		revent.URL = event.PullRequest.HTMLURL

diff --git a/pkg/provider/gitea/acl_test.go b/pkg/provider/gitea/acl_test.go
@@ -9,6 +9,7 @@ import (
 
 	"code.gitea.io/sdk/gitea"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/params"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/info"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings"
 	giteaStructs "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitea/structs"
@@ -79,9 +80,11 @@ func TestCheckPolicyAllowing(t *testing.T) {
 			ctx, _ := rtesting.SetupFakeContext(t)
 			observer, _ := zapobserver.New(zap.InfoLevel)
 			logger := zap.New(observer).Sugar()
-			repo := &v1alpha1.Repository{Spec: v1alpha1.RepositorySpec{
-				Settings: &v1alpha1.Settings{},
-			}}
+			repo := &v1alpha1.Repository{
+				Spec: v1alpha1.RepositorySpec{
+					Settings: &v1alpha1.Settings{},
+				},
+			}
 			gprovider := Provider{
 				Client: fakeclient,
 				repo:   repo,
@@ -286,14 +289,17 @@ func TestOkToTestComment(t *testing.T) {
 			gprovider := Provider{
 				Client: fakeclient,
 				Logger: logger,
-			}
-			pacopts := info.PacOpts{
-				Settings: &settings.Settings{
-					RememberOKToTest: tt.rememberOkToTest,
+				run: &params.Run{
+					Info: info.Info{
+						Pac: &info.PacOpts{
+							Settings: &settings.Settings{
+								RememberOKToTest: tt.rememberOkToTest,
+							},
+						},
+					},
 				},
 			}
-
-			isAllowed, err := gprovider.IsAllowed(ctx, &tt.runevent, &pacopts)
+			isAllowed, err := gprovider.IsAllowed(ctx, &tt.runevent)
 			if tt.wantErr {
 				assert.Assert(t, err != nil)
 			} else {

diff --git a/pkg/provider/gitea/gitea.go b/pkg/provider/gitea/gitea.go
@@ -48,6 +48,7 @@ type Provider struct {
 	Password     string
 	repo         *v1alpha1.Repository
 	eventEmitter *events.EventEmitter
+	run          *params.Run
 }
 
 // GetTaskURI TODO: Implement ME
@@ -82,7 +83,7 @@ func (v *Provider) GetConfig() *info.ProviderConfig {
 	}
 }
 
-func (v *Provider) SetClient(_ context.Context, _ *params.Run, runevent *info.Event, repo *v1alpha1.Repository, emitter *events.EventEmitter) error {
+func (v *Provider) SetClient(_ context.Context, run *params.Run, runevent *info.Event, repo *v1alpha1.Repository, emitter *events.EventEmitter) error {
 	var err error
 	apiURL := runevent.Provider.URL
 	// password is not exposed to CRD, it's only used from the e2e tests
@@ -100,6 +101,7 @@ func (v *Provider) SetClient(_ context.Context, _ *params.Run, runevent *info.Ev
 	v.giteaInstanceURL = runevent.Provider.URL
 	v.eventEmitter = emitter
 	v.repo = repo
+	v.run = run
 	return nil
 }
 

diff --git a/pkg/provider/github/acl.go b/pkg/provider/github/acl.go
@@ -61,7 +61,7 @@ func (v *Provider) IsAllowedOwnersFile(ctx context.Context, event *info.Event) (
 	return acl.UserInOwnerFile(ownerContent, event.Sender)
 }
 
-func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, _ *info.PacOpts) (bool, error) {
+func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	aclPolicy := policy.Policy{
 		Repository:   v.repo,
 		EventEmitter: v.eventEmitter,

diff --git a/pkg/provider/github/acl_test.go b/pkg/provider/github/acl_test.go
@@ -333,7 +333,7 @@ func TestOkToTestComment(t *testing.T) {
 				Run:           &params.Run{Info: info.Info{Pac: pacopts}},
 			}
 
-			got, err := gprovider.IsAllowed(ctx, &tt.runevent, pacopts)
+			got, err := gprovider.IsAllowed(ctx, &tt.runevent)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("aclCheck() error = %v, wantErr %v", err, tt.wantErr)
 				return

diff --git a/pkg/provider/gitlab/acl.go b/pkg/provider/gitlab/acl.go
@@ -65,7 +65,7 @@ func (v *Provider) checkOkToTestCommentFromApprovedMember(ctx context.Context, e
 	return false, nil
 }
 
-func (v *Provider) IsAllowed(ctx context.Context, event *info.Event, _ *info.PacOpts) (bool, error) {
+func (v *Provider) IsAllowed(ctx context.Context, event *info.Event) (bool, error) {
 	if v.Client == nil {
 		return false, fmt.Errorf("no github client has been initialized, " +
 			"exiting... (hint: did you forget setting a secret on your repo?)")

diff --git a/pkg/provider/gitlab/acl_test.go b/pkg/provider/gitlab/acl_test.go
@@ -115,8 +115,7 @@ func TestIsAllowed(t *testing.T) {
 
 				defer tearDown()
 			}
-			pacopts := info.PacOpts{}
-			got, err := v.IsAllowed(ctx, tt.args.event, &pacopts)
+			got, err := v.IsAllowed(ctx, tt.args.event)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("IsAllowed() error = %v, wantErr %v", err, tt.wantErr)
 				return

diff --git a/pkg/provider/interface.go b/pkg/provider/interface.go
@@ -30,7 +30,7 @@ type Interface interface {
 	Validate(ctx context.Context, params *params.Run, event *info.Event) error
 	Detect(*http.Request, string, *zap.SugaredLogger) (bool, bool, *zap.SugaredLogger, string, error)
 	ParsePayload(context.Context, *params.Run, *http.Request, string) (*info.Event, error)
-	IsAllowed(context.Context, *info.Event, *info.PacOpts) (bool, error)
+	IsAllowed(context.Context, *info.Event) (bool, error)
 	IsAllowedOwnersFile(context.Context, *info.Event) (bool, error)
 	CreateStatus(context.Context, versioned.Interface, *info.Event, *info.PacOpts, StatusOpts) error
 	GetTektonDir(context.Context, *info.Event, string, string) (string, error)      // ctx, event, path, provenance

diff --git a/pkg/test/provider/testwebvcs.go b/pkg/test/provider/testwebvcs.go
@@ -65,7 +65,7 @@ func (v *TestProviderImp) SetClient(_ context.Context, _ *params.Run, _ *info.Ev
 	return nil
 }
 
-func (v *TestProviderImp) IsAllowed(_ context.Context, _ *info.Event, _ *info.PacOpts) (bool, error) {
+func (v *TestProviderImp) IsAllowed(_ context.Context, _ *info.Event) (bool, error) {
 	if v.AllowIT {
 		return true, nil
 	}