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.

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

Ignore ghost user #53

Merged
merged 2 commits into from
Oct 31, 2020
Merged
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
38 changes: 9 additions & 29 deletions internal/check/valid_owner.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package check
import (
"context"
"net/http"
"net/mail"
"strings"

"github.com/mszostok/codeowners-validator/internal/ctxutil"
Expand Down Expand Up @@ -82,11 +81,11 @@ func (v *ValidOwner) Check(ctx context.Context, in Input) (Output, error) {

func (v *ValidOwner) selectValidateFn(name string) func(context.Context, string) *validateError {
switch {
case isGithubUser(name):
case IsGithubUser(name):
return v.validateGithubUser
case isGithubTeam(name):
case IsGithubTeam(name):
return v.validateTeam
case isEmailAddress(name):
case IsEmailAddress(name):
// TODO(mszostok): try to check if e-mail really exists
return func(context.Context, string) *validateError { return nil }
default:
Expand Down Expand Up @@ -162,6 +161,12 @@ func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateErr
}

func (v *ValidOwner) validateGithubUser(ctx context.Context, name string) *validateError {
// Ignore @ghost user
// https://git.luolix.topmunity/t5/How-to-use-Git-and-GitHub/CODEOWNERS-file-with-a-NOT-file-type-condition/m-p/31013/highlight/true#M8523
if IsGithubGhostUser(name) {
return nil
}

if v.orgMembers == nil { //TODO(mszostok): lazy init, make it more robust.
if err := v.initOrgListMembers(ctx); err != nil {
return newValidateError("Cannot initialize organization member list: %v", err).AsPermanent()
Expand Down Expand Up @@ -222,31 +227,6 @@ func (v *ValidOwner) initOrgListMembers(ctx context.Context) error {
return nil
}

func isEmailAddress(s string) bool {
_, err := mail.ParseAddress(s)

return err == nil
}

func isGithubTeam(s string) bool {
hasPrefix := strings.HasPrefix(s, "@")
containsSlash := strings.Contains(s, "/")
split := strings.SplitN(s, "/", 3) // 3 is enough to confirm that is invalid + will not overflow the buffer

if hasPrefix && containsSlash && len(split) == 2 {
return true
}

return false
}

func isGithubUser(s string) bool {
if strings.HasPrefix(s, "@") && !strings.Contains(s, "/") {
return true
}
return false
}

// Name returns human readable name of the validator
func (ValidOwner) Name() string {
return "Valid Owner Checker"
Expand Down
26 changes: 26 additions & 0 deletions internal/check/valid_owner_check.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package check

import (
"net/mail"
"strings"
)

func IsEmailAddress(s string) bool {
_, err := mail.ParseAddress(s)
return err == nil
}

func IsGithubTeam(s string) bool {
hasPrefix := strings.HasPrefix(s, "@")
containsSlash := strings.Contains(s, "/")
split := strings.SplitN(s, "/", 3) // 3 is enough to confirm that is invalid + will not overflow the buffer
return hasPrefix && containsSlash && len(split) == 2 && len(split[1]) > 0
}

func IsGithubUser(s string) bool {
return !strings.Contains(s, "/") && strings.HasPrefix(s, "@")
}

func IsGithubGhostUser(s string) bool {
return s == "@ghost"
}
52 changes: 52 additions & 0 deletions internal/check/valid_owner_check_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
package check_test

import (
"testing"

"github.com/mszostok/codeowners-validator/internal/check"

"github.com/stretchr/testify/assert"
)

func TestValidOwnerChecker(t *testing.T) {
tests := map[string]struct {
user string
isValid bool
}{
"Invalid Email": {
user: `asda.comm`,
isValid: false,
},
"Valid Email": {
user: `gmail@gmail.com`,
isValid: true,
},
"Invalid Team": {
user: `@org/`,
isValid: false,
},
"Valid Team": {
user: `@org/user`,
isValid: true,
},
"Invalid User": {
user: `user`,
isValid: false,
},
"Valid User": {
user: `@user`,
isValid: true,
},
}
for tn, tc := range tests {
t.Run(tn, func(t *testing.T) {
// when
result := isValidUser(tc.user)
assert.Equal(t, tc.isValid, result)
})
}
}

func isValidUser(user string) bool {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
func isValidUser(user string) bool {
func isValidOwner(user string) bool {

owner is a generic name for users, teams and emails :)

return check.IsEmailAddress(user) || check.IsGithubUser(user) || check.IsGithubTeam(user)
}