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

Cleanup init check logic, require configuration only if needed #26

Merged
merged 1 commit into from
Mar 15, 2020
Merged

Cleanup init check logic, require configuration only if needed #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions internal/check/api.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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"
}
Loading