Skip to content

Commit

Permalink
Cleanup init check logic, require configuration only if needed (#26)
Browse files Browse the repository at this point in the history
  • Loading branch information
@mszostok
mszostok committed Mar 15, 2020
1 parent 62b4264 commit cac5de0
Show file tree
Hide file tree
Showing 9 changed files with 215 additions and 152 deletions.
8 changes: 4 additions & 4 deletions internal/check/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ type (
}
)

type Opt func(*Issue)
type ReportIssueOpt func(*Issue)

func WithSeverity(s SeverityType) Opt {
func WithSeverity(s SeverityType) ReportIssueOpt {
return func(i *Issue) {
i.Severity = s
}
}

func WithEntry(e codeowners.Entry) Opt {
func WithEntry(e codeowners.Entry) ReportIssueOpt {
return func(i *Issue) {
i.LineNo = uint64Ptr(e.LineNo)
}
Expand All @@ -50,7 +50,7 @@ func uint64Ptr(u uint64) *uint64 {
return &c
}

func (out *Output) ReportIssue(msg string, opts ...Opt) Issue {
func (out *Output) ReportIssue(msg string, opts ...ReportIssueOpt) Issue {
if out == nil { // TODO: error?
return Issue{}
}
Expand Down
14 changes: 7 additions & 7 deletions internal/check/duplicated_pattern.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ import (
"github.com/mszostok/codeowners-validator/pkg/codeowners"
)

// DuplicatedPatternChecker validates if CODEOWNERS file does not contain
// DuplicatedPattern validates if CODEOWNERS file does not contain
// the duplicated lines with the same file pattern.
type DuplicatedPatternChecker struct{}
type DuplicatedPattern struct{}

// NewDuplicatedPattern returns instance of the DuplicatedPatternChecker
func NewDuplicatedPattern() *DuplicatedPatternChecker {
return &DuplicatedPatternChecker{}
// NewDuplicatedPattern returns instance of the DuplicatedPattern
func NewDuplicatedPattern() *DuplicatedPattern {
return &DuplicatedPattern{}
}

// Check searches for doubles paths(patterns) in CODEOWNERS file.
func (d *DuplicatedPatternChecker) Check(ctx context.Context, in Input) (Output, error) {
func (d *DuplicatedPattern) Check(ctx context.Context, in Input) (Output, error) {
var output Output

// TODO(mszostok): decide if the `CodeownerEntries` entry by default should be
Expand Down Expand Up @@ -55,6 +55,6 @@ func ListFormatFunc(es []codeowners.Entry) string {
}

// Name returns human readable name of the validator.
func (DuplicatedPatternChecker) Name() string {
func (DuplicatedPattern) Name() string {
return "Duplicated Pattern Checker"
}
10 changes: 5 additions & 5 deletions internal/check/file_exists.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import (
"github.com/pkg/errors"
)

type FileExistChecker struct{}
type FileExist struct{}

func NewFileExist() *FileExistChecker {
return &FileExistChecker{}
func NewFileExist() *FileExist {
return &FileExist{}
}

func (FileExistChecker) Check(ctx context.Context, in Input) (Output, error) {
func (FileExist) Check(ctx context.Context, in Input) (Output, error) {
var output Output

for _, entry := range in.CodeownerEntries {
Expand All @@ -38,6 +38,6 @@ func (FileExistChecker) Check(ctx context.Context, in Input) (Output, error) {
return output, nil
}

func (FileExistChecker) Name() string {
func (FileExist) Name() string {
return "File Exist Checker"
}
30 changes: 15 additions & 15 deletions internal/check/not_owned_file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,26 @@ import (
ctxutil "github.com/mszostok/codeowners-validator/internal/context"
)

type NotOwnedFileCheckerConfig struct {
type NotOwnedFileConfig struct {
SkipPatterns []string `envconfig:"optional"`
}

type NotOwnedFileChecker struct {
type NotOwnedFile struct {
skipPatterns map[string]struct{}
}

func NewNotOwnedFile(cfg NotOwnedFileCheckerConfig) *NotOwnedFileChecker {
func NewNotOwnedFile(cfg NotOwnedFileConfig) *NotOwnedFile {
skip := map[string]struct{}{}
for _, p := range cfg.SkipPatterns {
skip[p] = struct{}{}
}

return &NotOwnedFileChecker{
return &NotOwnedFile{
skipPatterns: skip,
}
}

func (c *NotOwnedFileChecker) Check(ctx context.Context, in Input) (output Output, err error) {
func (c *NotOwnedFile) Check(ctx context.Context, in Input) (output Output, err error) {
patterns, err := c.patternsToBeIgnored(ctx, in.CodeownerEntries)
if err != nil {
return Output{}, err
Expand Down Expand Up @@ -72,13 +72,13 @@ func (c *NotOwnedFileChecker) Check(ctx context.Context, in Input) (output Outpu
if lsOut != "" {
lines := strings.Split(lsOut, "\n")
msg := fmt.Sprintf("Found %d not owned files (skipped patterns: %q): \n%s", len(lines), c.skipPatternsList(), c.ListFormatFunc(lines))
output.ReportIssue(msg, WithSeverity(Warning))
output.ReportIssue(msg)
}

return output, nil
}

func (c *NotOwnedFileChecker) patternsToBeIgnored(ctx context.Context, entries []codeowners.Entry) ([]string, error) {
func (c *NotOwnedFile) patternsToBeIgnored(ctx context.Context, entries []codeowners.Entry) ([]string, error) {
var patterns []string
for _, entry := range entries {
if ctxutil.ShouldExit(ctx) {
Expand All @@ -94,7 +94,7 @@ func (c *NotOwnedFileChecker) patternsToBeIgnored(ctx context.Context, entries [
return patterns, nil
}

func (c *NotOwnedFileChecker) AppendToGitignoreFile(repoDir string, patterns []string) error {
func (c *NotOwnedFile) AppendToGitignoreFile(repoDir string, patterns []string) error {
f, err := os.OpenFile(path.Join(repoDir, ".gitignore"),
os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
Expand All @@ -115,7 +115,7 @@ func (c *NotOwnedFileChecker) AppendToGitignoreFile(repoDir string, patterns []s
return nil
}

func (c *NotOwnedFileChecker) GitRemoveIgnoredFiles(repoDir string) error {
func (c *NotOwnedFile) GitRemoveIgnoredFiles(repoDir string) error {
gitrm := pipe.Script(
pipe.ChDir(repoDir),
pipe.Line(
Expand All @@ -131,7 +131,7 @@ func (c *NotOwnedFileChecker) GitRemoveIgnoredFiles(repoDir string) error {
return nil
}

func (c *NotOwnedFileChecker) GitCheckStatus(repoDir string) error {
func (c *NotOwnedFile) GitCheckStatus(repoDir string) error {
gitstate := pipe.Script(
pipe.ChDir(repoDir),
pipe.Exec("git", "status", "--porcelain"),
Expand All @@ -149,7 +149,7 @@ func (c *NotOwnedFileChecker) GitCheckStatus(repoDir string) error {
return nil
}

func (c *NotOwnedFileChecker) GitResetCurrentBranch(repoDir string) error {
func (c *NotOwnedFile) GitResetCurrentBranch(repoDir string) error {
gitreset := pipe.Script(
pipe.ChDir(repoDir),
pipe.Exec("git", "reset", "--hard"),
Expand All @@ -161,7 +161,7 @@ func (c *NotOwnedFileChecker) GitResetCurrentBranch(repoDir string) error {
return nil
}

func (c *NotOwnedFileChecker) GitListFiles(repoDir string) (string, error) {
func (c *NotOwnedFile) GitListFiles(repoDir string) (string, error) {
gitls := pipe.Script(
pipe.ChDir(repoDir),
pipe.Exec("git", "ls-files"),
Expand All @@ -175,7 +175,7 @@ func (c *NotOwnedFileChecker) GitListFiles(repoDir string) (string, error) {
return string(stdout), nil
}

func (c *NotOwnedFileChecker) skipPatternsList() string {
func (c *NotOwnedFile) skipPatternsList() string {
list := make([]string, 0, len(c.skipPatterns))
for k := range c.skipPatterns {
list = append(list, k)
Expand All @@ -185,7 +185,7 @@ func (c *NotOwnedFileChecker) skipPatternsList() string {

// ListFormatFunc is a basic formatter that outputs
// a bullet point list of the pattern.
func (c *NotOwnedFileChecker) ListFormatFunc(es []string) string {
func (c *NotOwnedFile) ListFormatFunc(es []string) string {
points := make([]string, len(es))
for i, err := range es {
points[i] = fmt.Sprintf(" * %s", err)
Expand All @@ -195,6 +195,6 @@ func (c *NotOwnedFileChecker) ListFormatFunc(es []string) string {
}

// Name returns human readable name of the validator
func (NotOwnedFileChecker) Name() string {
func (NotOwnedFile) Name() string {
return "[Experimental] Not Owned File Checker"
}
65 changes: 29 additions & 36 deletions internal/check/valid_owner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package check

import (
"context"
"fmt"
"net/http"
"net/mail"
"strings"
Expand All @@ -11,27 +10,27 @@ import (
ctxutil "github.com/mszostok/codeowners-validator/internal/context"
)

type ValidOwnerCheckerConfig struct {
OrganizationName string
type ValidOwnerConfig struct {
OrganizationName string `envconfig:"optional"`
}

// ValidOwnerChecker validates each owner
type ValidOwnerChecker struct {
// ValidOwner validates each owner
type ValidOwner struct {
ghClient *github.Client
orgMembers *map[string]struct{}
orgName string
orgTeams map[string][]*github.Team
}

// NewValidOwner returns new instance of the ValidOwnerChecker
func NewValidOwner(cfg ValidOwnerCheckerConfig, ghClient *github.Client) *ValidOwnerChecker {
return &ValidOwnerChecker{
// NewValidOwner returns new instance of the ValidOwner
func NewValidOwner(cfg ValidOwnerConfig, ghClient *github.Client) *ValidOwner {
return &ValidOwner{
ghClient: ghClient,
orgName: cfg.OrganizationName,
}
}

// Check check if defined owners are the valid ones.
// Check checks if defined owners are the valid ones.
// Allowed owner syntax:
// @username
// @org/team-name
Expand All @@ -43,7 +42,7 @@ func NewValidOwner(cfg ValidOwnerCheckerConfig, ghClient *github.Client) *ValidO
// - if github user then check if have github account
// - if github user then check if he/she is in organization
// - if org team then check if exists in organization
func (v *ValidOwnerChecker) Check(ctx context.Context, in Input) (Output, error) {
func (v *ValidOwner) Check(ctx context.Context, in Input) (Output, error) {
var output Output
checkedOwners := map[string]struct{}{}

Expand All @@ -59,8 +58,8 @@ func (v *ValidOwnerChecker) Check(ctx context.Context, in Input) (Output, error)

validFn := v.selectValidateFn(ownerName)
if err := validFn(ctx, ownerName); err != nil {
output.ReportIssue(err.Msg, WithSeverity(err.Severity), WithEntry(entry))
if err.RateLimitReached { // Doesn't make sense to process further. TODO(mszostok): change for more generic solution like, `IsPermanentError`
output.ReportIssue(err.msg, WithEntry(entry))
if err.rateLimitReached { // Doesn't make sense to process further. TODO(mszostok): change for more generic solution like, `IsPermanentError`
return output, nil
}
}
Expand All @@ -71,7 +70,7 @@ func (v *ValidOwnerChecker) Check(ctx context.Context, in Input) (Output, error)
return output, nil
}

func (v *ValidOwnerChecker) selectValidateFn(name string) func(context.Context, string) *validateError {
func (v *ValidOwner) selectValidateFn(name string) func(context.Context, string) *validateError {
switch {
case isGithubUser(name):
return v.validateGithubUser
Expand All @@ -82,18 +81,12 @@ func (v *ValidOwnerChecker) selectValidateFn(name string) func(context.Context,
return func(context.Context, string) *validateError { return nil }
default:
return func(_ context.Context, name string) *validateError {
return &validateError{fmt.Sprintf("Not valid owner definition %q", name), Error, false}
return newValidateError("Not valid owner definition %q", name)
}
}
}

type validateError struct {
Msg string
Severity SeverityType
RateLimitReached bool
}

func (v *ValidOwnerChecker) initOrgListTeams(ctx context.Context, org string) ([]*github.Team, *validateError) {
func (v *ValidOwner) initOrgListTeams(ctx context.Context, org string) ([]*github.Team, *validateError) {
var teams []*github.Team
req := &github.ListOptions{
PerPage: 100,
Expand All @@ -104,13 +97,13 @@ func (v *ValidOwnerChecker) initOrgListTeams(ctx context.Context, org string) ([
switch err := err.(type) {
case *github.ErrorResponse:
if err.Response.StatusCode == http.StatusUnauthorized {
return nil, &validateError{fmt.Sprintf("Teams for org %q could not be queried. Requires GitHub authorization.", org), Warning, false}
return nil, newValidateError("Teams for organization %q could not be queried. Requires GitHub authorization.", org)
}
return nil, &validateError{fmt.Sprintf("HTTP error occurred while calling GitHub: %v", err), Error, false}
return nil, newValidateError("HTTP error occurred while calling GitHub: %v", err)
case *github.RateLimitError:
return nil, &validateError{fmt.Sprintf("GitHub rate limit reached: %v", err.Message), Warning, true}
return nil, newValidateError("GitHub rate limit reached: %v", err.Message).RateLimitReached()
default:
return nil, &validateError{fmt.Sprintf("Unknown error occurred while calling GitHub: %v", err), Error, false}
return nil, newValidateError("Unknown error occurred while calling GitHub: %v", err)
}
}
teams = append(teams, resultPage...)
Expand All @@ -128,7 +121,7 @@ func (v *ValidOwnerChecker) initOrgListTeams(ctx context.Context, org string) ([
return teams, nil
}

func (v *ValidOwnerChecker) validateTeam(ctx context.Context, name string) *validateError {
func (v *ValidOwner) validateTeam(ctx context.Context, name string) *validateError {
parts := strings.SplitN(name, "/", 2)
org := parts[0]
org = strings.TrimPrefix(org, "@")
Expand All @@ -153,16 +146,16 @@ func (v *ValidOwnerChecker) validateTeam(ctx context.Context, name string) *vali
}

if !teamExists() {
return &validateError{fmt.Sprintf("Team %q does not exist in organization %q or has no permissions associated with the repository.", team, org), Warning, false}
return newValidateError("Team %q does not exist in organization %q or has no permissions associated with the repository.", team, org)
}

return nil
}

func (v *ValidOwnerChecker) validateGithubUser(ctx context.Context, name string) *validateError {
func (v *ValidOwner) validateGithubUser(ctx context.Context, name string) *validateError {
if v.orgMembers == nil { //TODO(mszostok): lazy init, make it more robust.
if err := v.initOrgListMembers(ctx); err != nil {
return &validateError{fmt.Sprintf("Cannot initialize organization member list: %v", err), Error, false}
return newValidateError("Cannot initialize organization member list: %v", err)
}
}

Expand All @@ -172,19 +165,19 @@ func (v *ValidOwnerChecker) validateGithubUser(ctx context.Context, name string)
switch err := err.(type) {
case *github.ErrorResponse:
if err.Response.StatusCode == http.StatusNotFound {
return &validateError{fmt.Sprintf("User %q does not have github account", name), Error, false}
return newValidateError("User %q does not have github account", name)
}
return &validateError{fmt.Sprintf("HTTP error occurred while calling GitHub: %v", err), Error, false}
return newValidateError("HTTP error occurred while calling GitHub: %v", err)
case *github.RateLimitError:
return &validateError{fmt.Sprintf("GitHub rate limit reached: %v", err.Message), Warning, true}
return newValidateError("GitHub rate limit reached: %v", err.Message).RateLimitReached()
default:
return &validateError{fmt.Sprintf("Unknown error occurred while calling GitHub: %v", err), Error, false}
return newValidateError("Unknown error occurred while calling GitHub: %v", err)
}
}

_, isMember := (*v.orgMembers)[userName]
if !isMember {
return &validateError{fmt.Sprintf("User %q is not a member of the organization", name), Error, false}
return newValidateError("User %q is not a member of the organization", name)
}

return nil
Expand All @@ -194,7 +187,7 @@ func (v *ValidOwnerChecker) validateGithubUser(ctx context.Context, name string)
// client.Organizations.IsMember(context.Background(), "org-name", "user-name")
// But latency is too huge for checking each single user independent
// better and faster is to ask for all members and cache them.
func (v *ValidOwnerChecker) initOrgListMembers(ctx context.Context) error {
func (v *ValidOwner) initOrgListMembers(ctx context.Context) error {
opt := &github.ListMembersOptions{
ListOptions: github.ListOptions{PerPage: 100},
}
Expand Down Expand Up @@ -246,6 +239,6 @@ func isGithubUser(s string) bool {
}

// Name returns human readable name of the validator
func (ValidOwnerChecker) Name() string {
func (ValidOwner) Name() string {
return "Valid Owner Checker"
}

0 comments on commit cac5de0

Please sign in to comment.