feat: add codeowner validate api (#733)

pull/3423/merge
Abhinav Singh 2023-10-30 22:16:39 +00:00 committed by Harness
parent 081d79717a
commit e0df722ce3
12 changed files with 331 additions and 139 deletions

View File

@ -41,30 +41,18 @@ func (c *Controller) CodeOwners(
return types.CodeOwnerEvaluation{}, fmt.Errorf("failed to get pull request by number: %w", err)
}
filteredCodeOwners, err := c.codeOwners.GetApplicableCodeOwnersForPR(ctx, repo, pr)
reviewers, err := c.reviewerStore.List(ctx, pr.ID)
if err != nil {
return types.CodeOwnerEvaluation{}, fmt.Errorf("failed to get reviewers by pr: %w", err)
}
ownerEvaluation, err := c.codeOwners.Evaluate(ctx, repo, pr, reviewers)
if errors.Is(codeowners.ErrNotFound, err) {
return types.CodeOwnerEvaluation{}, usererror.ErrNotFound
}
if codeowners.IsTooLargeError(err) {
return types.CodeOwnerEvaluation{}, usererror.UnprocessableEntityf(err.Error())
}
if err != nil {
return types.CodeOwnerEvaluation{}, fmt.Errorf("failed to get codeOwners: %w", err)
}
if len(filteredCodeOwners.Entries) == 0 {
return types.CodeOwnerEvaluation{
EvaluationEntries: nil,
FileSha: filteredCodeOwners.FileSHA,
}, nil
}
reviewers, err := c.reviewerStore.List(ctx, pullreqNum)
if err != nil {
return types.CodeOwnerEvaluation{}, fmt.Errorf("failed to get reviewers by pr: %w", err)
}
ownerEvaluation, err := c.codeOwners.Evaluate(ctx, filteredCodeOwners, reviewers)
if err != nil {
return types.CodeOwnerEvaluation{}, err
}

View File

@ -144,17 +144,10 @@ func (c *Controller) Merge(
return nil, nil, fmt.Errorf("failed to fetch protection rules for the repository: %w", err)
}
ownersForPR, err := c.codeOwners.GetApplicableCodeOwnersForPR(ctx, sourceRepo, pr)
if codeowners.IsTooLargeError(err) {
return nil, nil, usererror.UnprocessableEntityf(err.Error())
}
codeOwnerWithApproval, err := c.codeOwners.Evaluate(ctx, sourceRepo, pr, reviewers)
// check for error and ignore if it is codeowners file not found else throw error
if err != nil && !errors.Is(err, codeowners.ErrNotFound) {
return nil, nil, fmt.Errorf("failed to find codeOwners for PR: %w", err)
}
codeOwnerWithApproval, err := c.codeOwners.Evaluate(ctx, ownersForPR, reviewers)
if err != nil {
return nil, nil, fmt.Errorf("failed to get code owners with approval: %w", err)
return nil, nil, fmt.Errorf("CODEOWNERS evaluation failed: %w", err)
}
ruleOut, violations, err := protectionRules.MergeVerify(ctx, protection.MergeVerifyInput{

View File

@ -0,0 +1,46 @@
// Copyright 2023 Harness, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package repo
import (
"context"
"github.com/harness/gitness/app/auth"
"github.com/harness/gitness/types"
"github.com/harness/gitness/types/enum"
)
func (c *Controller) CodeOwnersValidate(
ctx context.Context,
session *auth.Session,
repoRef string,
ref string,
) (*types.CodeOwnersValidation, error) {
repo, err := c.getRepoCheckAccess(ctx, session, repoRef, enum.PermissionRepoView, true)
if err != nil {
return nil, err
}
if ref == "" {
ref = repo.DefaultBranch
}
violations, err := c.codeOwners.Validate(ctx, repo, ref)
if err != nil {
return nil, err
}
return violations, nil
}

View File

@ -0,0 +1,45 @@
// Copyright 2023 Harness, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package repo
import (
"net/http"
"github.com/harness/gitness/app/api/controller/repo"
"github.com/harness/gitness/app/api/render"
"github.com/harness/gitness/app/api/request"
)
func HandleCodeOwnersValidate(repoCtrl *repo.Controller) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
session, _ := request.AuthSessionFrom(ctx)
repoRef, err := request.GetRepoRefFromPath(r)
if err != nil {
render.TranslatedUserError(w, err)
return
}
ref := request.GetGitRefFromQueryOrDefault(r, "")
violations, err := repoCtrl.CodeOwnersValidate(ctx, session, repoRef, ref)
if err != nil {
render.TranslatedUserError(w, err)
return
}
render.JSON(w, http.StatusOK, violations)
}
}

View File

@ -157,6 +157,10 @@ type getRawDiffRequest struct {
Range string `path:"range" example:"main..dev"`
}
type codeOwnersValidate struct {
repoRequest
}
// ruleType is a plugin for types.RuleType to allow using oneof.
type ruleType string
@ -819,4 +823,17 @@ func repoOperations(reflector *openapi3.Reflector) {
_ = reflector.SetJSONResponse(&opRuleGet, new(usererror.Error), http.StatusForbidden)
_ = reflector.SetJSONResponse(&opRuleGet, new(usererror.Error), http.StatusNotFound)
_ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/rules/{rule_uid}", opRuleGet)
opCodeOwnerValidate := openapi3.Operation{}
opCodeOwnerValidate.WithTags("repository")
opCodeOwnerValidate.WithMapOfAnything(map[string]interface{}{"operationId": "codeOwnersValidate"})
opCodeOwnerValidate.WithParameters(queryParameterGitRef)
_ = reflector.SetRequest(&opCodeOwnerValidate, new(codeOwnersValidate), http.MethodGet)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, nil, http.StatusOK)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, new(usererror.Error), http.StatusInternalServerError)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, new(usererror.Error), http.StatusUnprocessableEntity)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, new(usererror.Error), http.StatusUnauthorized)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, new(usererror.Error), http.StatusForbidden)
_ = reflector.SetJSONResponse(&opCodeOwnerValidate, new(usererror.Error), http.StatusNotFound)
_ = reflector.Spec.AddOperation(http.MethodGet, "/repos/{repo_ref}/codeowners/validate", opCodeOwnerValidate)
}

View File

@ -19,6 +19,7 @@ import (
"net/http"
apiauth "github.com/harness/gitness/app/api/auth"
"github.com/harness/gitness/app/services/codeowners"
"github.com/harness/gitness/app/services/webhook"
"github.com/harness/gitness/blob"
"github.com/harness/gitness/gitrpc"
@ -30,10 +31,11 @@ import (
func Translate(err error) *Error {
var (
rError *Error
checkError *check.ValidationError
gitrpcError *gitrpc.Error
maxBytesErr *http.MaxBytesError
rError *Error
checkError *check.ValidationError
gitrpcError *gitrpc.Error
maxBytesErr *http.MaxBytesError
codeOwnersTooLargeError *codeowners.TooLargeError
)
// TODO: Improve performance of checking multiple errors with errors.Is
@ -87,6 +89,12 @@ func Translate(err error) *Error {
case errors.Is(err, webhook.ErrWebhookNotRetriggerable):
return ErrWebhookNotRetriggerable
// codeowners errors
case errors.Is(err, codeowners.ErrNotFound):
return ErrCodeOwnersNotFound
case errors.As(err, &codeOwnersTooLargeError):
return UnprocessableEntityf(codeOwnersTooLargeError.Error())
// unknown error
default:
log.Warn().Msgf("Unable to translate error: %s", err)

View File

@ -76,6 +76,9 @@ var (
// ErrWebhookNotRetriggerable is returned if the webhook can't be retriggered.
ErrWebhookNotRetriggerable = New(http.StatusMethodNotAllowed,
"The webhook execution is incomplete and can't be retriggered")
// ErrCodeOwnersNotFound is returned when codeowners file is not found.
ErrCodeOwnersNotFound = New(http.StatusNotFound, "CODEOWNERS file not found")
)
// Error represents a json-encoded API error.

View File

@ -314,6 +314,8 @@ func setupRepos(r chi.Router,
r.Post("/*", handlerrepo.HandleMergeCheck(repoCtrl))
})
r.Get("/codeowners/validate", handlerrepo.HandleCodeOwnersValidate(repoCtrl))
SetupPullReq(r, pullreqCtrl)
SetupWebhook(r, webhookCtrl)

View File

@ -231,7 +231,7 @@ func (s *Service) getCodeOwnerFile(
}, nil
}
func (s *Service) GetApplicableCodeOwnersForPR(
func (s *Service) getApplicableCodeOwnersForPR(
ctx context.Context,
repo *types.Repository,
pr *types.PullReq,
@ -252,7 +252,7 @@ func (s *Service) GetApplicableCodeOwnersForPR(
}
for _, entry := range codeOwners.Entries {
ok, err := contains(diffFileStats.Files, entry.Pattern)
ok, err := contains(entry.Pattern, diffFileStats.Files)
if err != nil {
return nil, err
}
@ -268,24 +268,31 @@ func (s *Service) GetApplicableCodeOwnersForPR(
func (s *Service) Evaluate(
ctx context.Context,
owners *CodeOwners,
repo *types.Repository,
pr *types.PullReq,
reviewer []*types.PullReqReviewer,
) (*Evaluation, error) {
if owners == nil {
owners, err := s.getApplicableCodeOwnersForPR(ctx, repo, pr)
if err != nil {
return &Evaluation{}, fmt.Errorf("failed to get codeOwners: %w", err)
}
if owners == nil || len(owners.Entries) == 0 {
return &Evaluation{}, nil
}
flattenedReviewers := flattenReviewers(reviewer)
evaluationEntries := make([]EvaluationEntry, len(owners.Entries))
for i, entry := range owners.Entries {
ownerEvaluations := make([]OwnerEvaluation, len(entry.Owners))
for j, owner := range entry.Owners {
ownerEvaluations := make([]OwnerEvaluation, 0, len(owners.Entries))
for _, owner := range entry.Owners {
if pullreqReviewer, ok := flattenedReviewers[owner]; ok {
ownerEvaluations[j] = OwnerEvaluation{
ownerEvaluations = append(ownerEvaluations, OwnerEvaluation{
Owner: pullreqReviewer.Reviewer,
ReviewDecision: pullreqReviewer.ReviewDecision,
ReviewSHA: pullreqReviewer.SHA,
}
})
continue
}
principal, err := s.principalStore.FindByEmail(ctx, owner)
@ -297,9 +304,9 @@ func (s *Service) Evaluate(
if err != nil {
return &Evaluation{}, fmt.Errorf("error finding user by email: %w", err)
}
ownerEvaluations[j] = OwnerEvaluation{
ownerEvaluations = append(ownerEvaluations, OwnerEvaluation{
Owner: *principal.ToPrincipalInfo(),
}
})
}
evaluationEntries[i] = EvaluationEntry{
Pattern: entry.Pattern,
@ -313,6 +320,49 @@ func (s *Service) Evaluate(
}, nil
}
func (s *Service) Validate(
ctx context.Context,
repo *types.Repository,
branch string,
) (*types.CodeOwnersValidation, error) {
var codeOwnerValidation types.CodeOwnersValidation
// check file parsing, existence and size
codeowners, err := s.get(ctx, repo, branch)
if err != nil {
return nil, err
}
for _, entry := range codeowners.Entries {
// check for users in file
for _, owner := range entry.Owners {
_, err := s.principalStore.FindByEmail(ctx, owner)
if errors.Is(err, gitness_store.ErrResourceNotFound) {
codeOwnerValidation.Addf(enum.CodeOwnerViolationCodeUserNotFound,
"user %q not found", owner)
continue
}
if err != nil {
return nil, fmt.Errorf("error encountered fetching user %q by email: %w", owner, err)
}
}
// check for pattern
if entry.Pattern == "" {
codeOwnerValidation.Add(enum.CodeOwnerViolationCodePatternEmpty,
"empty pattern")
continue
}
ok := doublestar.ValidatePathPattern(entry.Pattern)
if !ok {
codeOwnerValidation.Addf(enum.CodeOwnerViolationCodePatternInvalid, "pattern %q is invalid",
entry.Pattern)
}
}
return &codeOwnerValidation, nil
}
func flattenReviewers(reviewers []*types.PullReqReviewer) map[string]*types.PullReqReviewer {
r := make(map[string]*types.PullReqReviewer)
for _, reviewer := range reviewers {
@ -326,8 +376,8 @@ func flattenReviewers(reviewers []*types.PullReqReviewer) map[string]*types.Pull
// A path foo/bar will match against pattern ** or foo/*
// Also, for a directory ending with / we have to return true for all files in that directory,
// hence we append ** for it.
func contains(patterns []string, target string) (bool, error) {
for _, pattern := range patterns {
func contains(pattern string, targets []string) (bool, error) {
for _, target := range targets {
// in case of / ending rule, owner owns the whole directory hence append **
if strings.HasSuffix(pattern, "/") {
pattern += "**"

View File

@ -100,24 +100,27 @@ func TestService_ParseCodeOwner(t *testing.T) {
}
func Test_contains(t *testing.T) {
pattern1 := [1]string{"*"}
pattern2 := [1]string{"**"}
pattern3 := [1]string{"abc/xyz"}
pattern4 := [2]string{"abc/xyz", "*"}
pattern5 := [2]string{"abc/xyz", "**"}
pattern6 := [1]string{"doc/frotz"}
pattern7 := [1]string{"?ilename"}
pattern8 := [1]string{"**/foo"}
pattern9 := [1]string{"foo/**"}
pattern10 := [1]string{"a/**/b"}
pattern11 := [1]string{"foo/*"}
pattern12 := [1]string{"*.txt"}
pattern13 := [1]string{"/scripts/"}
target1 := []string{"random"}
target2 := []string{"random/xyz"}
target3 := []string{"abhinav/path"}
target4 := []string{"abc/xyz"}
target5 := []string{"abc/xyz", "random"}
target6 := []string{"doc/frotz"}
target7 := []string{"filename"}
target8 := []string{"as/foo"}
target9 := []string{"foo/bar"}
target10 := []string{"a/x/y/b"}
target11 := []string{"foo/getting-started.md"}
target12 := []string{"foo.txt"}
target13 := []string{"/scripts/filename.txt"}
target14 := []string{"path/to/file.txt"}
target15 := []string{"path/to/foo"}
target16 := []string{"foo/build-app/troubleshooting.md"}
type args struct {
ctx context.Context
slice []string
target string
ctx context.Context
pattern string
target []string
}
tests := []struct {
name string
@ -127,178 +130,151 @@ func Test_contains(t *testing.T) {
{
name: "Test * pattern",
args: args{
ctx: nil,
slice: pattern1[:],
target: "random",
ctx: nil,
pattern: "*",
target: target1,
},
want: true,
},
{
name: "Test ** pattern",
args: args{
ctx: nil,
slice: pattern2[:],
target: "random/xyz",
ctx: nil,
pattern: "**",
target: target2,
},
want: true,
},
{
name: "Test ** pattern on fixed path",
args: args{
ctx: nil,
slice: pattern1[:],
target: "abhinav/path",
ctx: nil,
pattern: "abc/xyz",
target: target3,
},
want: false,
},
{
name: "Test abc/xyz pattern",
args: args{
ctx: nil,
slice: pattern3[:],
target: "abc/xyz",
ctx: nil,
pattern: "abc/xyz",
target: target4,
},
want: true,
},
{
name: "Test abc/xyz pattern negative",
args: args{
ctx: nil,
slice: pattern3[:],
target: "abc/xy",
},
want: false,
},
{
name: "Test incorrect pattern negative",
args: args{
ctx: nil,
slice: pattern4[:],
target: "random/path",
ctx: nil,
pattern: "random/xyz",
target: target5,
},
want: false,
},
{
name: "Test * pattern with bigger slice",
args: args{
ctx: nil,
slice: pattern4[:],
target: "random",
},
want: true,
},
{
name: "Test file path with **",
args: args{
ctx: nil,
slice: pattern5[:],
target: "path/to/file",
ctx: nil,
pattern: "**",
target: target14,
},
want: true,
},
{
name: "Test / pattern",
args: args{
ctx: nil,
slice: pattern6[:],
target: "doc/frotz",
ctx: nil,
pattern: "doc/frotz",
target: target6,
},
want: true,
},
{
name: "Test ? pattern",
args: args{
ctx: nil,
slice: pattern7[:],
target: "filename",
ctx: nil,
pattern: "?ilename",
target: target7,
},
want: true,
},
{
name: "Test /** pattern",
args: args{
ctx: nil,
slice: pattern8[:],
target: "foo",
ctx: nil,
pattern: "**/foo",
target: target8,
},
want: true,
},
{
name: "Test /** pattern with slash",
args: args{
ctx: nil,
slice: pattern8[:],
target: "foo/bar",
},
want: false,
},
{
name: "Test **/ with deep nesting",
args: args{
ctx: nil,
slice: pattern8[:],
target: "path/to/foo",
ctx: nil,
pattern: "**/foo",
target: target15,
},
want: true,
},
{
name: "Test **/ pattern",
args: args{
ctx: nil,
slice: pattern9[:],
target: "foo/bar",
ctx: nil,
pattern: "foo/**",
target: target9,
},
want: true,
},
{
name: "Test a/**/b pattern",
args: args{
ctx: nil,
slice: pattern10[:],
target: "a/x/y/b",
ctx: nil,
pattern: "a/x/y/b",
target: target10,
},
want: true,
},
{
name: "Test /* pattern positive",
args: args{
ctx: nil,
slice: pattern11[:],
target: "foo/getting-started.md",
ctx: nil,
pattern: "foo/*",
target: target11,
},
want: true,
},
{
name: "Test /* pattern negative",
args: args{
ctx: nil,
slice: pattern11[:],
target: "foo/build-app/troubleshooting.md",
ctx: nil,
pattern: "foo/*",
target: target16,
},
want: false,
},
{
name: "Test * for files",
args: args{
ctx: nil,
slice: pattern12[:],
target: "foo.txt",
ctx: nil,
pattern: "*.txt",
target: target12,
},
want: true,
},
{
name: "Test /a/",
args: args{
ctx: nil,
slice: pattern13[:],
target: "/scripts/filename.txt",
ctx: nil,
pattern: "/scripts/",
target: target13,
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got, _ := contains(tt.args.slice, tt.args.target); got != tt.want {
if got, _ := contains(tt.args.pattern, tt.args.target); got != tt.want {
t.Errorf("contains() = %v, want %v", got, tt.want)
}
})

View File

@ -14,7 +14,11 @@
package types
import "github.com/harness/gitness/types/enum"
import (
"fmt"
"github.com/harness/gitness/types/enum"
)
type CodeOwnerEvaluation struct {
EvaluationEntries []CodeOwnerEvaluationEntry `json:"evaluation_entries"`
@ -28,6 +32,32 @@ type CodeOwnerEvaluationEntry struct {
type OwnerEvaluation struct {
Owner PrincipalInfo `json:"owner"`
ReviewDecision enum.PullReqReviewDecision `json:"review_decision,omitempty"`
ReviewSHA string `json:"review_sha,omitempty"`
ReviewDecision enum.PullReqReviewDecision `json:"review_decision"`
ReviewSHA string `json:"review_sha"`
}
type CodeOwnersValidation struct {
Violations []CodeOwnersViolation `json:"violations"`
}
type CodeOwnersViolation struct {
Code enum.CodeOwnerViolationCode `json:"code"`
Message string `json:"message"`
Params []any `json:"params"`
}
func (violations *CodeOwnersValidation) Add(code enum.CodeOwnerViolationCode, message string) {
violations.Violations = append(violations.Violations, CodeOwnersViolation{
Code: code,
Message: message,
Params: nil,
})
}
func (violations *CodeOwnersValidation) Addf(code enum.CodeOwnerViolationCode, format string, params ...any) {
violations.Violations = append(violations.Violations, CodeOwnersViolation{
Code: code,
Message: fmt.Sprintf(format, params...),
Params: params,
})
}

View File

@ -0,0 +1,34 @@
// Copyright 2023 Harness, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package enum
type CodeOwnerViolationCode string
const (
// CodeOwnerViolationCodeUserNotFound occurs when user in codeowners file is not present.
CodeOwnerViolationCodeUserNotFound CodeOwnerViolationCode = "user_not_found"
// CodeOwnerViolationCodePatternInvalid occurs when a pattern in codeowners file is incorrect.
CodeOwnerViolationCodePatternInvalid CodeOwnerViolationCode = "pattern_invalid"
// CodeOwnerViolationCodePatternEmpty occurs when a pattern in codeowners file is empty.
CodeOwnerViolationCodePatternEmpty CodeOwnerViolationCode = "pattern_empty"
)
func (CodeOwnerViolationCode) Enum() []interface{} { return toInterfaceSlice(codeOwnerViolationCodes) }
var codeOwnerViolationCodes = sortEnum([]CodeOwnerViolationCode{
CodeOwnerViolationCodeUserNotFound,
CodeOwnerViolationCodePatternInvalid,
CodeOwnerViolationCodePatternEmpty,
})