diff --git a/.travis.yml b/.travis.yml index 45e3818..88718d1 100644 --- a/.travis.yml +++ b/.travis.yml @@ -16,15 +16,15 @@ script: skip jobs: include: - stage: test - name: "Build and unit-test with Go 1.12.x" + name: "Build and unit-test with Go 1.14.x" go: 1.14.x script: make test-unit - stage: test - name: "Build and unit-test with Go 1.13.x" + name: "Build and unit-test with Go 1.15.x" go: 1.15.x script: make test-unit - stage: test - name: "Hammer unit-test with Go 1.13.x" + name: "Hammer unit-test with Go 1.15.x" go: 1.15.x script: make test-hammer - stage: test diff --git a/README.md b/README.md index e57151f..320dcd4 100644 --- a/README.md +++ b/README.md @@ -88,7 +88,7 @@ You can also download [latest version](https://github.com/mszostok/codeowners-va You can install `codeowners-validator` with `env GO111MODULE=on go get -u github.com/mszostok/codeowners-validator`. -> NOTE: please use the latest Go to do this, ideally Go 1.12 or greater. +> NOTE: please use the latest Go to do this, ideally Go 1.15 or greater. This will put `codeowners-validator` in `$(go env GOPATH)/bin` @@ -118,7 +118,8 @@ The following checks are enabled by default: |------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | duppatterns | **[Duplicated Pattern Checker]**

Reports if CODEOWNERS file contain duplicated lines with the same file pattern. | | files | **[File Exist Checker]**

Reports if CODEOWNERS file contain lines with the file pattern that do not exist in a given repository. | -| owners | **[Valid Owner Checker]**

Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `user@example.com`
_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_.

**Checks:**
1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address).

2. Check if a GitHub owner has a GitHub account

3. Check if a GitHub owner is in a given organization

4. Check if an organization team exists | +| owners | **[Valid Owner Checker]**

Reports if CODEOWNERS file contain invalid owners definition. Allowed owner syntax: `@username`, `@org/team-name` or `user@example.com`
_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_.

**Checks:**
    1. Check if the owner's definition is valid (is either a GitHub user name, an organization team name or an email address).

    2. Check if a GitHub owner has a GitHub account

    3. Check if a GitHub owner is in a given organization

    4. Check if an organization team exists | +| syntax | **[Valid Syntax Checker]**

Reports if CODEOWNERS file contain invalid syntax definition. It is imported as:
    "If any line in your CODEOWNERS file contains invalid syntax, the file will not be detected
    and will not be used to request reviews. Invalid syntax includes inline comments
    and user or team names that do not exist on GitHub."

_source: https://help.github.com/articles/about-code-owners/#codeowners-syntax_. | The experimental checks are disabled by default: diff --git a/internal/check/valid_owner.go b/internal/check/valid_owner.go index eace92e..85b6fde 100644 --- a/internal/check/valid_owner.go +++ b/internal/check/valid_owner.go @@ -39,7 +39,7 @@ func NewValidOwner(cfg ValidOwnerConfig, ghClient *github.Client) (*ValidOwner, }, nil } -// Check checks if defined owners are the valid ones. +// Check if defined owners are the valid ones. // Allowed owner syntax: // @username // @org/team-name diff --git a/internal/check/valid_syntax.go b/internal/check/valid_syntax.go new file mode 100644 index 0000000..1f3f657 --- /dev/null +++ b/internal/check/valid_syntax.go @@ -0,0 +1,77 @@ +package check + +import ( + "context" + "fmt" + "regexp" + "strings" + + "github.com/mszostok/codeowners-validator/internal/ctxutil" +) + +var ( + // A valid username/organization name has up to 39 characters (per GitHub Join page) + // and is matched by the following regex: /^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i + // A valid team name consists of alphanumerics, underscores and dashes + usernameOrTeamRegexp = regexp.MustCompile(`^@(?i:[a-z\d](?:[a-z\d-]){0,37}[a-z\d](/[a-z\d](?:[a-z\d_-]*)[a-z\d])?)$`) + + // Per: https://davidcel.is/posts/stop-validating-email-addresses-with-regex/ + // just check if there is '@' and a '.' afterwards + emailRegexp = regexp.MustCompile(`.+@.+\..+`) +) + +// ValidSyntax provides a syntax validation for CODEOWNERS file. +// +// If any line in your CODEOWNERS file contains invalid syntax, the file will not be detected and will +// not be used to request reviews. Invalid syntax includes inline comments and user or team names that do not exist on GitHub. +type ValidSyntax struct{} + +// NewValidSyntax returns new ValidSyntax instance. +func NewValidSyntax() *ValidSyntax { + return &ValidSyntax{} +} + +// Check for syntax issues in your CODEOWNERS file. +func (ValidSyntax) Check(ctx context.Context, in Input) (Output, error) { + var bldr OutputBuilder + + for _, entry := range in.CodeownersEntries { + if ctxutil.ShouldExit(ctx) { + return Output{}, ctx.Err() + } + + if entry.Pattern == "" { + bldr.ReportIssue("Missing pattern", WithEntry(entry)) + } + + if len(entry.Owners) == 0 { + bldr.ReportIssue("Missing owner, at least one owner is required", WithEntry(entry)) + } + + ownersLoop: + for _, item := range entry.Owners { + switch { + case strings.EqualFold(item, "#"): + msg := "Comment (# sign) is not allowed in line with pattern entry. The correct format is: pattern owner1 ... ownerN" + bldr.ReportIssue(msg, WithEntry(entry)) + break ownersLoop // no need to check for the rest items in this line, as the whole line is already marked as invalid + case strings.HasPrefix(item, "@"): + if !usernameOrTeamRegexp.MatchString(item) { + msg := fmt.Sprintf("Owner '%s' does not look like a GitHub username or team name", item) + bldr.ReportIssue(msg, WithEntry(entry), WithSeverity(Warning)) + } + default: + if !emailRegexp.MatchString(item) { + msg := fmt.Sprintf("Owner '%s' does not look like an email", item) + bldr.ReportIssue(msg, WithEntry(entry)) + } + } + } + } + + return bldr.Output(), nil +} + +func (ValidSyntax) Name() string { + return "Valid Syntax Checker" +} diff --git a/internal/check/valid_syntax_test.go b/internal/check/valid_syntax_test.go new file mode 100644 index 0000000..051b71c --- /dev/null +++ b/internal/check/valid_syntax_test.go @@ -0,0 +1,125 @@ +package check_test + +import ( + "context" + "testing" + + "github.com/mszostok/codeowners-validator/internal/check" + "github.com/mszostok/codeowners-validator/internal/ptr" + "github.com/mszostok/codeowners-validator/pkg/codeowners" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidSyntaxChecker(t *testing.T) { + tests := map[string]struct { + codeowners string + issue check.Issue + }{ + "No owners": { + codeowners: `*`, + issue: check.Issue{ + Severity: check.Error, + LineNo: ptr.Uint64Ptr(1), + Message: "Missing owner, at least one owner is required", + }, + }, + "Bad username": { + codeowners: `pkg/github.com/** @-`, + issue: check.Issue{ + Severity: check.Warning, + LineNo: ptr.Uint64Ptr(1), + Message: "Owner '@-' does not look like a GitHub username or team name", + }, + }, + "Bad org": { + codeowners: `* @bad+org`, + issue: check.Issue{ + Severity: check.Warning, + LineNo: ptr.Uint64Ptr(1), + Message: "Owner '@bad+org' does not look like a GitHub username or team name", + }, + }, + "Bad team name on first place": { + codeowners: `* @org/+not+a+good+name`, + issue: check.Issue{ + Severity: check.Warning, + LineNo: ptr.Uint64Ptr(1), + Message: "Owner '@org/+not+a+good+name' does not look like a GitHub username or team name", + }, + }, + "Bad team name on second place": { + codeowners: `* @org/hakuna-matata @org/-a-team`, + issue: check.Issue{ + Severity: check.Warning, + LineNo: ptr.Uint64Ptr(1), + Message: "Owner '@org/-a-team' does not look like a GitHub username or team name", + }, + }, + "Doesn't look like username, team name, nor email": { + codeowners: `* something_weird`, + issue: check.Issue{ + Severity: check.Error, + LineNo: ptr.Uint64Ptr(1), + Message: "Owner 'something_weird' does not look like an email", + }, + }, + "Comment in pattern line": { + codeowners: `* @org/hakuna-matata # this should be an error`, + issue: check.Issue{ + Severity: check.Error, + LineNo: ptr.Uint64Ptr(1), + Message: "Comment (# sign) is not allowed in line with pattern entry. The correct format is: pattern owner1 ... ownerN", + }, + }, + } + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // when + out, err := check.NewValidSyntax(). + Check(context.Background(), loadInput(tc.codeowners)) + + // then + require.NoError(t, err) + + require.Len(t, out.Issues, 1) + assert.EqualValues(t, tc.issue, out.Issues[0]) + }) + } +} + +func TestValidSyntaxZeroValueEntry(t *testing.T) { + // given + zeroValueInput := check.Input{ + CodeownersEntries: []codeowners.Entry{ + { + LineNo: 0, + Pattern: "", + Owners: nil, + }, + }, + } + expIssues := []check.Issue{ + { + LineNo: ptr.Uint64Ptr(0), + Severity: check.Error, + Message: "Missing pattern", + }, + { + LineNo: ptr.Uint64Ptr(0), + Severity: check.Error, + Message: "Missing owner, at least one owner is required", + }, + } + + // when + out, err := check.NewValidSyntax(). + Check(context.Background(), zeroValueInput) + + // then + require.NoError(t, err) + + require.Len(t, out.Issues, 2) + assert.EqualValues(t, expIssues, out.Issues) +} diff --git a/internal/load/load.go b/internal/load/load.go index d66d831..aba0968 100644 --- a/internal/load/load.go +++ b/internal/load/load.go @@ -17,6 +17,10 @@ import ( func Checks(ctx context.Context, enabledChecks []string, experimentalChecks []string) ([]check.Checker, error) { var checks []check.Checker + if isEnabled(enabledChecks, "syntax") { + checks = append(checks, check.NewValidSyntax()) + } + if isEnabled(enabledChecks, "duppatterns") { checks = append(checks, check.NewDuplicatedPattern()) } diff --git a/pkg/codeowners/owners.go b/pkg/codeowners/owners.go index 5dd03be..fb4f1fb 100644 --- a/pkg/codeowners/owners.go +++ b/pkg/codeowners/owners.go @@ -6,7 +6,6 @@ import ( "io" "os" "path" - "regexp" "strings" "github.com/spf13/afero" @@ -33,7 +32,7 @@ func NewFromPath(path string) ([]Entry, error) { return nil, err } - return parseCodeowners(r) + return ParseCodeowners(r), nil } // openCodeownersFile finds a CODEOWNERS file and returns content. @@ -66,57 +65,22 @@ func openCodeownersFile(dir string) (io.Reader, error) { return nil, fmt.Errorf("No CODEOWNERS found in the root, docs/, or .github/ directory of the repository %s", dir) } -func parseCodeowners(r io.Reader) ([]Entry, error) { +func ParseCodeowners(r io.Reader) []Entry { var e []Entry - - usernameOrTeamRegexp := regexp.MustCompile(`^@(?i:[a-z\d](?:[a-z\d-]){0,37}[a-z\d](/[a-z\d](?:[a-z\d_-]*)[a-z\d])?)$`) - emailRegexp := regexp.MustCompile(`.+@.+\..+`) - s := bufio.NewScanner(r) no := uint64(0) for s.Scan() { no++ + fields := strings.Fields(s.Text()) - line := s.Text() - if strings.HasPrefix(line, "#") { // comment + if len(fields) == 0 { // empty continue } - if len(line) == 0 { // empty + if strings.HasPrefix(fields[0], "#") { // comment continue } - fields := strings.Fields(s.Text()) - - if len(fields) < 2 { - return e, fmt.Errorf("line %d does not have 2 or more fields", no) - } - - // This does syntax validation only - // - // Syntax check: all fields are valid team/username identifiers or emails - // Allowed owner syntax: - // @username - // @org/team-name - // user@example.com - // source: https://help.github.com/articles/about-code-owners/#codeowners-syntax - for _, entry := range fields[1:] { - if strings.HasPrefix(entry, "@") { - // A valid username/organization name has up to 39 characters (per GitHub Join page) - // and is matched by the following regex: /^[a-z\d](?:[a-z\d]|-(?=[a-z\d])){0,38}$/i - // A valid team name consists of alphanumerics, underscores and dashes - if !usernameOrTeamRegexp.MatchString(entry) { - return e, fmt.Errorf("entry '%s' on line %d does not look like a GitHub username or team name", entry, no) - } - } else { - // Per: https://davidcel.is/posts/stop-validating-email-addresses-with-regex/ - // just check if there is '@' and a '.' afterwards - if !emailRegexp.MatchString(entry) { - return e, fmt.Errorf("entry '%s' on line %d does not look like an email", entry, no) - } - } - } - e = append(e, Entry{ Pattern: fields[0], Owners: fields[1:], @@ -124,5 +88,5 @@ func parseCodeowners(r io.Reader) ([]Entry, error) { }) } - return e, nil + return e } diff --git a/pkg/codeowners/owners_test.go b/pkg/codeowners/owners_test.go index 169523f..a23b502 100644 --- a/pkg/codeowners/owners_test.go +++ b/pkg/codeowners/owners_test.go @@ -20,51 +20,6 @@ pkg/github.com/** @myk ` -func TestParseCodeownersFailure(t *testing.T) { - // given - givenCodeownerPath := "workspace/go/repo-name" - badInputs := []string{ - `# one token -* -`, - `# bad username -* @myk -pkg/github.com/** @- -`, - `# bad org -* @bad+org -`, - `# comment mid line -* @org/hakuna-matata # this should be an error -`, - `# bad team name second place -* @org/hakuna-matata @org/-a-team -`, - `# bad team first -* @org/+not+a+good+name -`, - `# doesn't look like username, team name, nor email -* something_weird -`, - } - - for _, input := range badInputs { - tFS := afero.NewMemMapFs() - revert := codeowners.SetFS(tFS) - defer revert() - - f, _ := tFS.Create(path.Join(givenCodeownerPath, "CODEOWNERS")) - _, err := f.WriteString(input) - require.NoError(t, err) - - // when - _, err = codeowners.NewFromPath(givenCodeownerPath) - - // then - require.Error(t, err) - } -} - func TestParseCodeownersSuccess(t *testing.T) { // given givenCodeownerPath := "workspace/go/repo-name"