[githubgen] PR feedback follow-ups (#655)

* add template for repo name Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix tests, add chlog Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add default code owner flag and use it Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * move constants to separate file Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * update deprecated github client Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * extract function Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * make func more testable, generate test file Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add happy path test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * mock getgithubmembers Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * refactoring, make unit test more better Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * refactoring with unit test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * make github org for membership check configurable, make path suffix trimming configurable Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add back to versions.yaml Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add more cmd line flag defaults Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * adjust changelog msg Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add better method doc comment Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * update readme with new flags Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * make precommit Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix templates Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix markdown line length Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * remove pretty repo name from input flags and file templates Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * remove otel specific membership hint from file templates, remove hardcoded otel github org usage Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * Update githubgen/README.md Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com> * exchange confmap provider metadata reading for normal yaml unmarshal * move constant * use default code owners instead of exiting with error when a component doesnt have codeowners * handle file read/writing relative to the configured folder * linter Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * distributions unit test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * add license header Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * refactoring plus test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * typo Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * another unit test Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * adapt codeowners file to latest PR Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * adapt error message Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> * fix linter issue Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> --------- Signed-off-by: Moritz Wiesinger <moritz.wiesinger@dynatrace.com> Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
open-telemetry · Jan 22, 2025 · e1e1314 · e1e1314
1 parent b29324c
commit e1e1314
Show file tree

Hide file tree

Showing 15 changed files with 598 additions and 215 deletions.
diff --git a/.chloggen/githubgen-enhancements.yaml b/.chloggen/githubgen-enhancements.yaml
@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: 'enhancement'
+
+# The name of the component, or a single word describing the area of concern, (e.g. crosslink)
+component: githubgen
+
+# A brief description of the change.  Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Enhanced githubgen tool with more options to better fit arbitrary repos, added unit tests
+
+# One or more tracking issues related to the change
+issues: [655]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:
diff --git a/githubgen/README.md b/githubgen/README.md
@@ -17,7 +17,9 @@ $> ./githubgen
 The equivalent of:
 
 ```shell
-$> GITHUB_TOKEN=<mypattoken> githubgen --folder . [--allowlist cmd/githubgen/allowlist.txt] 
+$> githubgen --skipgithub --folder . --github-org "open-telemetry" \
+  --default-codeowner open-telemetry/opentelemetry-collector-approvers \
+  --allowlist cmd/githubgen/allowlist.txt
 ```
 
 ## Checking codeowners against OpenTelemetry membership via GitHub API
@@ -26,12 +28,16 @@ To authenticate, set the environment variable `GITHUB_TOKEN` to a PAT token.
 If a PAT is not available you can use the `--skipgithub` flag to avoid checking
 for membership in the GitHub organization.
 
+Additionally, the GitHub organization name needs to be specified using
+`--github-org <your-org>`, along with `--default-codeowner` to be used as the
+default owner for general files.
+
 For each codeowner, the script will check if the user is registered as a member
 of the OpenTelemetry organization.
 
 If any codeowner is missing, it will stop and print names of missing codeowners.
 
 These can be added to allowlist.txt as a workaround.
 
-If a codeowner is present in allowlist.txt and also a member of the
+If a codeowner is present in `allowlist.txt` and also a member of the
 OpenTelemetry organization, the script will error out.
diff --git a/githubgen/codeowners.go b/githubgen/codeowners.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"slices"
 	"sort"
 	"strings"
 
@@ -15,142 +16,45 @@ import (
 	"go.opentelemetry.io/build-tools/githubgen/datatype"
 )
 
-const allowlistHeader = `# Code generated by githubgen. DO NOT EDIT.
-#####################################################
-#
-# List of components in OpenTelemetry Collector Contrib
-# waiting on owners to be assigned
-#
-#####################################################
-#
-# Learn about membership in OpenTelemetry community:
-#  https://github.com/open-telemetry/community/blob/main/guides/contributor/membership.md
-#
-#
-# Learn about CODEOWNERS file format:
-#  https://help.github.com/en/articles/about-code-owners
-#
-
-## 
-# NOTE: New components MUST have one or more codeowners. Add codeowners to the component metadata.yaml and run make gengithub
-##
-
-## COMMON & SHARED components
-internal/common
-
-`
-
-const unmaintainedHeader = `
-
-## UNMAINTAINED components
-
-`
-
-const codeownersHeader = `# Code generated by githubgen. DO NOT EDIT.
-#####################################################
-#
-# List of codeowners for OpenTelemetry Collector Contrib
-#
-#####################################################
-#
-# Learn about membership in OpenTelemetry community:
-# https://github.com/open-telemetry/community/blob/main/guides/contributor/membership.md
-#
-#
-# Learn about CODEOWNERS file format:
-# https://help.github.com/en/articles/about-code-owners
-#
-
-* @open-telemetry/collector-contrib-approvers
-`
-
-const distributionCodeownersHeader = `
-#####################################################
-#
-# List of distribution maintainers for OpenTelemetry Collector Contrib
-#
-#####################################################
-`
-
 type codeownersGenerator struct {
-	skipGithub bool
+	skipGithub       bool
+	getGitHubMembers func(skipGithub bool, githubOrg string) (map[string]struct{}, error)
 }
 
 func (cg *codeownersGenerator) Generate(data datatype.GithubData) error {
 	allowlistData, err := os.ReadFile(data.AllowlistFilePath)
 	if err != nil {
 		return err
 	}
-	allowlistLines := strings.Split(string(allowlistData), "\n")
 
-	allowlist := make(map[string]struct{}, len(allowlistLines))
-	unusedAllowlist := make(map[string]struct{}, len(allowlistLines))
-	for _, line := range allowlistLines {
-		if line == "" {
-			continue
-		}
-		allowlist[line] = struct{}{}
-		unusedAllowlist[line] = struct{}{}
-	}
-	var missingCodeowners []string
-	var duplicateCodeowners []string
-	members, err := cg.getGithubMembers()
+	err = cg.verifyCodeOwnerOrgMembership(allowlistData, data)
 	if err != nil {
 		return err
 	}
-	for _, codeowner := range data.Codeowners {
-		_, present := members[codeowner]
-
-		if !present {
-			_, allowed := allowlist[codeowner]
-			delete(unusedAllowlist, codeowner)
-			allowed = allowed || strings.HasPrefix(codeowner, "open-telemetry/")
-			if !allowed {
-				missingCodeowners = append(missingCodeowners, codeowner)
-			}
-		} else if _, ok := allowlist[codeowner]; ok {
-			duplicateCodeowners = append(duplicateCodeowners, codeowner)
-		}
-	}
-	if len(missingCodeowners) > 0 && !cg.skipGithub {
-		sort.Strings(missingCodeowners)
-		return fmt.Errorf("codeowners are not members: %s", strings.Join(missingCodeowners, ", "))
-	}
-	if len(duplicateCodeowners) > 0 {
-		sort.Strings(duplicateCodeowners)
-		return fmt.Errorf("codeowners members duplicate in allowlist: %s", strings.Join(duplicateCodeowners, ", "))
-	}
-	if len(unusedAllowlist) > 0 {
-		var unused []string
-		for k := range unusedAllowlist {
-			unused = append(unused, k)
-		}
-		sort.Strings(unused)
-		return fmt.Errorf("unused members in allowlist: %s", strings.Join(unused, ", "))
-	}
 
-	codeowners := codeownersHeader
-	deprecatedList := "## DEPRECATED components\n"
-	unmaintainedList := "\n## UNMAINTAINED components\n"
+	codeowners := fmt.Sprintf(codeownersHeader, data.DefaultCodeOwner)
+	deprecatedList := deprecatedListHeader
+	unmaintainedList := unmaintainedListHeader
 
 	unmaintainedCodeowners := unmaintainedHeader
 	currentFirstSegment := ""
+
 LOOP:
-	for _, key := range data.Folders {
-		m := data.Components[key]
+	for _, folder := range data.Folders {
+		m := data.Components[folder]
 		for stability := range m.Status.Stability {
 			if stability == unmaintainedStatus {
-				unmaintainedList += key + "/\n"
-				unmaintainedCodeowners += fmt.Sprintf("%s/%s @open-telemetry/collector-contrib-approvers\n", key, strings.Repeat(" ", data.MaxLength-len(key)))
+				unmaintainedList += folder + "/\n"
+				unmaintainedCodeowners += fmt.Sprintf("%s/%s %s\n", folder, strings.Repeat(" ", data.MaxLength-len(folder)), data.DefaultCodeOwner)
 				continue LOOP
 			}
 			if stability == "deprecated" && (m.Status.Codeowners == nil || len(m.Status.Codeowners.Active) == 0) {
-				deprecatedList += key + "/\n"
+				deprecatedList += folder + "/\n"
 			}
 		}
 
 		if m.Status.Codeowners != nil {
-			parts := strings.Split(key, string(os.PathSeparator))
+			parts := strings.Split(folder, string(os.PathSeparator))
 			firstSegment := parts[0]
 			if firstSegment != currentFirstSegment {
 				currentFirstSegment = firstSegment
@@ -159,59 +63,126 @@ LOOP:
 			owners := ""
 			for _, owner := range m.Status.Codeowners.Active {
 				owners += " "
-				owners += "@" + owner
+				if !strings.HasPrefix(owner, "@") {
+					owners += "@" + owner
+				}
 			}
-			codeowners += fmt.Sprintf("%s/%s @open-telemetry/collector-contrib-approvers%s\n", key, strings.Repeat(" ", data.MaxLength-len(key)), owners)
+			codeowners += fmt.Sprintf("%s/%s %s%s\n", strings.TrimPrefix(folder, data.RootFolder+"/"), strings.Repeat(" ", data.MaxLength-len(folder)), data.DefaultCodeOwner, owners)
 		}
 	}
 
 	codeowners += distributionCodeownersHeader
-	longestName := 0
-	for _, dist := range data.Distributions {
-		if longestName < len(dist.Name) {
-			longestName = len(dist.Name)
-		}
-	}
+	longestName := cg.longestNameSpaces(data)
 
 	for _, dist := range data.Distributions {
 		var maintainers []string
 		for _, m := range dist.Maintainers {
 			maintainers = append(maintainers, fmt.Sprintf("@%s", m))
 		}
 
-		distribution := fmt.Sprintf("\nreports/distributions/%s.yaml%s @open-telemetry/collector-contrib-approvers", dist.Name, strings.Repeat(" ", longestName-len(dist.Name)))
+		distribution := fmt.Sprintf("\nreports/distributions/%s.yaml%s %s", dist.Name, strings.Repeat(" ", longestName-len(dist.Name)), data.DefaultCodeOwner)
 		if len(maintainers) > 0 {
 			distribution += fmt.Sprintf(" %s", strings.Join(maintainers, " "))
 		}
 
 		codeowners += distribution
 	}
 
-	err = os.WriteFile(filepath.Join(".github", "CODEOWNERS"), []byte(codeowners+unmaintainedCodeowners), 0o600)
+	err = os.WriteFile(filepath.Join(data.RootFolder, ".github", "CODEOWNERS"), []byte(codeowners+unmaintainedCodeowners), 0o600)
 	if err != nil {
 		return err
 	}
-	err = os.WriteFile(filepath.Join(".github", "ALLOWLIST"), []byte(allowlistHeader+deprecatedList+unmaintainedList), 0o600)
+	err = os.WriteFile(filepath.Join(data.RootFolder, ".github", "ALLOWLIST"), []byte(allowlistHeader+deprecatedList+unmaintainedList), 0o600)
 	if err != nil {
 		return err
 	}
 	return nil
 }
 
-func (cg *codeownersGenerator) getGithubMembers() (map[string]struct{}, error) {
-	if cg.skipGithub {
+func (cg *codeownersGenerator) longestNameSpaces(data datatype.GithubData) int {
+	longestName := 0
+	for _, dist := range data.Distributions {
+		if longestName < len(dist.Name) {
+			longestName = len(dist.Name)
+		}
+	}
+	return longestName
+}
+
+// verifyCodeOwnerOrgMembership verifies that all codeOwners are members of the defined GitHub organization
+//
+// If a codeOwner is not part of the GitHub org, that user will be looked for in the allowlist.
+//
+// The method returns an error if:
+// - there are code owners that are not org members and not in the allowlist (only if skipGithub is set to false)
+// - there are redundant entries in the allowlist
+// - there are entries in the allowlist that are unused
+func (cg *codeownersGenerator) verifyCodeOwnerOrgMembership(allowlistData []byte, data datatype.GithubData) error {
+	allowlist := strings.Split(string(allowlistData), "\n")
+	allowlist = slices.DeleteFunc(allowlist, func(s string) bool {
+		return s == ""
+	})
+	unusedAllowlist := append([]string{}, allowlist...)
+
+	var missingCodeowners []string
+	var duplicateCodeowners []string
+
+	members, err := cg.getGitHubMembers(cg.skipGithub, data.GitHubOrg)
+	if err != nil {
+		return err
+	}
+
+	// sort codeowners
+	for _, codeowner := range data.Codeowners {
+		_, ownerPresentInMembers := members[codeowner]
+
+		if !ownerPresentInMembers {
+			ownerInAllowlist := slices.Contains(allowlist, codeowner)
+			unusedAllowlist = slices.DeleteFunc(unusedAllowlist, func(s string) bool {
+				return s == codeowner
+			})
+
+			ownerInAllowlist = ownerInAllowlist || strings.HasPrefix(codeowner, data.GitHubOrg+"/")
+
+			if !ownerInAllowlist {
+				missingCodeowners = append(missingCodeowners, codeowner)
+			}
+		} else if slices.Contains(allowlist, codeowner) {
+			duplicateCodeowners = append(duplicateCodeowners, codeowner)
+		}
+	}
+
+	// error cases
+	if len(missingCodeowners) > 0 && !cg.skipGithub {
+		sort.Strings(missingCodeowners)
+		return fmt.Errorf("codeowners are not members: %s", strings.Join(missingCodeowners, ", "))
+	}
+	if len(duplicateCodeowners) > 0 {
+		sort.Strings(duplicateCodeowners)
+		return fmt.Errorf("codeowners members duplicate in allowlist: %s", strings.Join(duplicateCodeowners, ", "))
+	}
+	if len(unusedAllowlist) > 0 {
+		unused := append([]string{}, unusedAllowlist...)
+		sort.Strings(unused)
+		return fmt.Errorf("unused members in allowlist: %s", strings.Join(unused, ", "))
+	}
+	return err
+}
+
+func GetGithubMembers(skipGithub bool, githubOrg string) (map[string]struct{}, error) {
+	if skipGithub {
 		// don't try to get organization members if no token is expected
 		return map[string]struct{}{}, nil
 	}
 	githubToken := os.Getenv("GITHUB_TOKEN")
 	if githubToken == "" {
 		return nil, fmt.Errorf("Set the environment variable `GITHUB_TOKEN` to a PAT token to authenticate")
 	}
-	client := github.NewTokenClient(context.Background(), githubToken)
+	client := github.NewClient(nil).WithAuthToken(githubToken)
 	var allUsers []*github.User
 	pageIndex := 0
 	for {
-		users, resp, err := client.Organizations.ListMembers(context.Background(), "open-telemetry",
+		users, resp, err := client.Organizations.ListMembers(context.Background(), githubOrg,
 			&github.ListMembersOptions{
 				PublicOnly: false,
 				ListOptions: github.ListOptions{