refactor: use coder/slog + minor go style changes (#107)

Changes are broken down in to multiples commits to hopefully make
reviewing easy. 1 commit for the slog change and then a commit per Go
file for style changes.

Style changes are generally:
- try to use full sentences for all comments
- try to stick to 120 column lines (not strict) instead of 80
- try to one line as many `call function, check if err != nil` blocks as
possible (ex: only err or variables are not reused outside the if statement)
- try to use `err` or `errs` for all return type names, previously used
`problems` in some cases but `errs` in others
- some minor readability changes
- `Todo` -> `TODO`, sometimes also useful to do `TODO (name):` to make
it easier to find things a specific author meant to follow up on
- comments for types/functions should generally start with `//
FunctionName/TypeName ...`

---------

Signed-off-by: Callum Styan <callumstyan@gmail.com>

Author: cstyan

cmd/readmevalidation/coderresources.go

@@ -2,8 +2,8 @@ package main
 
 import (
 	"bufio"
+	"context"
 	"errors"
-	"log"
 	"net/url"
 	"os"
 	"path"
@@ -15,7 +15,14 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
-var supportedResourceTypes = []string{"modules", "templates"}
+var (
+	supportedResourceTypes = []string{"modules", "templates"}
+
+	// TODO: This is a holdover from the validation logic used by the Coder Modules repo. It gives us some assurance, but
+	// realistically, we probably want to parse any Terraform code snippets, and make some deeper guarantees about how it's
+	// structured. Just validating whether it *can* be parsed as Terraform would be a big improvement.
+	terraformVersionRe = regexp.MustCompile(`^\s*\bversion\s+=`)
+)
 
 type coderResourceFrontmatter struct {
 	Description string   `yaml:"description"`
@@ -49,36 +56,36 @@ func validateCoderResourceDescription(description string) error {
 	return nil
 }
 
-func validateCoderResourceIconURL(iconURL string) []error {
-	problems := []error{}
+func isPermittedRelativeURL(checkURL string) bool {
+	// Would normally be skittish about having relative paths like this, but it should be safe because we have
+	// guarantees about the structure of the repo, and where this logic will run.
+	return strings.HasPrefix(checkURL, "./") || strings.HasPrefix(checkURL, "/") || strings.HasPrefix(checkURL, "../../../../.icons")
+}
 
+func validateCoderResourceIconURL(iconURL string) []error {
 	if iconURL == "" {
-		problems = append(problems, xerrors.New("icon URL cannot be empty"))
-		return problems
+		return []error{xerrors.New("icon URL cannot be empty")}
 	}
 
-	isAbsoluteURL := !strings.HasPrefix(iconURL, ".") && !strings.HasPrefix(iconURL, "/")
-	if isAbsoluteURL {
+	errs := []error{}
+
+	// If the URL does not have a relative path.
+	if !strings.HasPrefix(iconURL, ".") && !strings.HasPrefix(iconURL, "/") {
 		if _, err := url.ParseRequestURI(iconURL); err != nil {
-			problems = append(problems, xerrors.New("absolute icon URL is not correctly formatted"))
+			errs = append(errs, xerrors.New("absolute icon URL is not correctly formatted"))
 		}
 		if strings.Contains(iconURL, "?") {
-			problems = append(problems, xerrors.New("icon URLs cannot contain query parameters"))
+			errs = append(errs, xerrors.New("icon URLs cannot contain query parameters"))
 		}
-		return problems
+		return errs
 	}
 
-	// Would normally be skittish about having relative paths like this, but it
-	// should be safe because we have guarantees about the structure of the
-	// repo, and where this logic will run.
-	isPermittedRelativeURL := strings.HasPrefix(iconURL, "./") ||
-		strings.HasPrefix(iconURL, "/") ||
-		strings.HasPrefix(iconURL, "../../../../.icons")
-	if !isPermittedRelativeURL {
-		problems = append(problems, xerrors.Errorf("relative icon URL %q must either be scoped to that module's directory, or the top-level /.icons directory (this can usually be done by starting the path with \"../../../.icons\")", iconURL))
+	// If the URL has a relative path.
+	if !isPermittedRelativeURL(iconURL) {
+		errs = append(errs, xerrors.Errorf("relative icon URL %q must either be scoped to that module's directory, or the top-level /.icons directory (this can usually be done by starting the path with \"../../../.icons\")", iconURL))
 	}
 
-	return problems
+	return errs
 }
 
 func validateCoderResourceTags(tags []string) error {
@@ -89,9 +96,8 @@ func validateCoderResourceTags(tags []string) error {
 		return nil
 	}
 
-	// All of these tags are used for the module/template filter controls in the
-	// Registry site. Need to make sure they can all be placed in the browser
-	// URL without issue.
+	// All of these tags are used for the module/template filter controls in the Registry site. Need to make sure they
+	// can all be placed in the browser URL without issue.
 	invalidTags := []string{}
 	for _, t := range tags {
 		if t != url.QueryEscape(t) {
@@ -105,16 +111,11 @@ func validateCoderResourceTags(tags []string) error {
 	return nil
 }
 
-// Todo: This is a holdover from the validation logic used by the Coder Modules
-// repo. It gives us some assurance, but realistically, we probably want to
-// parse any Terraform code snippets, and make some deeper guarantees about how
-// it's structured. Just validating whether it *can* be parsed as Terraform
-// would be a big improvement.
-var terraformVersionRe = regexp.MustCompile(`^\s*\bversion\s+=`)
-
 func validateCoderResourceReadmeBody(body string) []error {
-	trimmed := strings.TrimSpace(body)
 	var errs []error
+
+	trimmed := strings.TrimSpace(body)
+	// TODO: this may cause unexpected behavior since the errors slice may have a 0 length. Add a test.
 	errs = append(errs, validateReadmeBody(trimmed)...)
 
 	foundParagraph := false
@@ -130,9 +131,8 @@ func validateCoderResourceReadmeBody(body string) []error {
 		lineNum++
 		nextLine := lineScanner.Text()
 
-		// Code assumes that invalid headers would've already been handled by
-		// the base validation function, so we don't need to check deeper if the
-		// first line isn't an h1.
+		// Code assumes that invalid headers would've already been handled by the base validation function, so we don't
+		// need to check deeper if the first line isn't an h1.
 		if lineNum == 1 {
 			if !strings.HasPrefix(nextLine, "# ") {
 				break
@@ -159,15 +159,13 @@ func validateCoderResourceReadmeBody(body string) []error {
 			continue
 		}
 
-		// Code assumes that we can treat this case as the end of the "h1
-		// section" and don't need to process any further lines.
+		// Code assumes that we can treat this case as the end of the "h1 section" and don't need to process any further lines.
 		if lineNum > 1 && strings.HasPrefix(nextLine, "#") {
 			break
 		}
 
-		// Code assumes that if we've reached this point, the only other options
-		// are: (1) empty spaces, (2) paragraphs, (3) HTML, and (4) asset
-		// references made via [] syntax.
+		// Code assumes that if we've reached this point, the only other options are:
+		// (1) empty spaces, (2) paragraphs, (3) HTML, and (4) asset references made via [] syntax.
 		trimmedLine := strings.TrimSpace(nextLine)
 		isParagraph := trimmedLine != "" && !strings.HasPrefix(trimmedLine, "![") && !strings.HasPrefix(trimmedLine, "<")
 		foundParagraph = foundParagraph || isParagraph
@@ -250,7 +248,7 @@ func parseCoderResourceReadmeFiles(resourceType string, rms []readme) (map[strin
 	}
 	if len(yamlParsingErrs) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseReadmeParsing,
+			phase:  validationPhaseReadme,
 			errors: yamlParsingErrs,
 		}
 	}
@@ -264,7 +262,7 @@ func parseCoderResourceReadmeFiles(resourceType string, rms []readme) (map[strin
 	}
 	if len(yamlValidationErrors) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseReadmeParsing,
+			phase:  validationPhaseReadme,
 			errors: yamlValidationErrors,
 		}
 	}
@@ -274,7 +272,7 @@ func parseCoderResourceReadmeFiles(resourceType string, rms []readme) (map[strin
 
 // Todo: Need to beef up this function by grabbing each image/video URL from
 // the body's AST.
-func validateCoderResourceRelativeUrls(_ map[string]coderResourceReadme) error {
+func validateCoderResourceRelativeURLs(_ map[string]coderResourceReadme) error {
 	return nil
 }
 
@@ -321,7 +319,7 @@ func aggregateCoderResourceReadmeFiles(resourceType string) ([]readme, error) {
 
 	if len(errs) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseFileLoad,
+			phase:  validationPhaseFile,
 			errors: errs,
 		}
 	}
@@ -338,17 +336,16 @@ func validateAllCoderResourceFilesOfType(resourceType string) error {
 		return err
 	}
 
-	log.Printf("Processing %d README files\n", len(allReadmeFiles))
+	logger.Info(context.Background(), "rocessing README files", "num_files", len(allReadmeFiles))
 	resources, err := parseCoderResourceReadmeFiles(resourceType, allReadmeFiles)
 	if err != nil {
 		return err
 	}
-	log.Printf("Processed %d README files as valid Coder resources with type %q", len(resources), resourceType)
+	logger.Info(context.Background(), "rocessed README files as valid Coder resources", "num_files", len(resources), "type", resourceType)
 
-	err = validateCoderResourceRelativeUrls(resources)
-	if err != nil {
+	if err := validateCoderResourceRelativeURLs(resources); err != nil {
 		return err
 	}
-	log.Printf("All relative URLs for %s READMEs are valid\n", resourceType)
+	logger.Info(context.Background(), "all relative URLs for READMEs are valid", "type", resourceType)
 	return nil
 }

cmd/readmevalidation/contributors.go

@@ -1,7 +1,7 @@
 package main
 
 import (
-	"log"
+	"context"
 	"net/url"
 	"os"
 	"path"
@@ -50,17 +50,16 @@ func validateContributorLinkedinURL(linkedinURL *string) error {
 	return nil
 }
 
+// validateContributorSupportEmail does best effort validation of a contributors email address.	We can't 100% validate
+// that this is correct without actually sending an email, especially because some contributors are individual developers
+// and we don't want to do that on every single run of the CI pipeline. The best we can do is verify the general structure.
 func validateContributorSupportEmail(email *string) []error {
 	if email == nil {
 		return nil
 	}
 
 	errs := []error{}
 
-	// Can't 100% validate that this is correct without actually sending
-	// an email, and especially with some contributors being individual
-	// developers, we don't want to do that on every single run of the CI
-	// pipeline. Best we can do is verify the general structure.
 	username, server, ok := strings.Cut(*email, "@")
 	if !ok {
 		errs = append(errs, xerrors.Errorf("email address %q is missing @ symbol", *email))
@@ -110,29 +109,26 @@ func validateContributorStatus(status string) error {
 	return nil
 }
 
-// Can't validate the image actually leads to a valid resource in a pure
-// function, but can at least catch obvious problems.
+// Can't validate the image actually leads to a valid resource in a pure function, but can at least catch obvious problems.
 func validateContributorAvatarURL(avatarURL *string) []error {
 	if avatarURL == nil {
 		return nil
 	}
 
-	errs := []error{}
 	if *avatarURL == "" {
-		errs = append(errs, xerrors.New("avatar URL must be omitted or non-empty string"))
-		return errs
+		return []error{xerrors.New("avatar URL must be omitted or non-empty string")}
 	}
 
-	// Have to use .Parse instead of .ParseRequestURI because this is the
-	// one field that's allowed to be a relative URL.
+	errs := []error{}
+	// Have to use .Parse instead of .ParseRequestURI because this is the one field that's allowed to be a relative URL.
 	if _, err := url.Parse(*avatarURL); err != nil {
 		errs = append(errs, xerrors.Errorf("URL %q is not a valid relative or absolute URL", *avatarURL))
 	}
 	if strings.Contains(*avatarURL, "?") {
 		errs = append(errs, xerrors.New("avatar URL is not allowed to contain search parameters"))
 	}
 
-	matched := false
+	var matched bool
 	for _, ff := range supportedAvatarFileFormats {
 		matched = strings.HasSuffix(*avatarURL, ff)
 		if matched {
@@ -210,22 +206,21 @@ func parseContributorFiles(readmeEntries []readme) (map[string]contributorProfil
 	}
 	if len(yamlParsingErrors) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseReadmeParsing,
+			phase:  validationPhaseReadme,
 			errors: yamlParsingErrors,
 		}
 	}
 
 	yamlValidationErrors := []error{}
 	for _, p := range profilesByNamespace {
-		errors := validateContributorReadme(p)
-		if len(errors) > 0 {
+		if errors := validateContributorReadme(p); len(errors) > 0 {
 			yamlValidationErrors = append(yamlValidationErrors, errors...)
 			continue
 		}
 	}
 	if len(yamlValidationErrors) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseReadmeParsing,
+			phase:  validationPhaseReadme,
 			errors: yamlValidationErrors,
 		}
 	}
@@ -241,12 +236,13 @@ func aggregateContributorReadmeFiles() ([]readme, error) {
 
 	allReadmeFiles := []readme{}
 	errs := []error{}
+	dirPath := ""
 	for _, e := range dirEntries {
-		dirPath := path.Join(rootRegistryPath, e.Name())
 		if !e.IsDir() {
 			continue
 		}
 
+		dirPath = path.Join(rootRegistryPath, e.Name())
 		readmePath := path.Join(dirPath, "README.md")
 		rmBytes, err := os.ReadFile(readmePath)
 		if err != nil {
@@ -261,26 +257,26 @@ func aggregateContributorReadmeFiles() ([]readme, error) {
 
 	if len(errs) != 0 {
 		return nil, validationPhaseError{
-			phase:  validationPhaseFileLoad,
+			phase:  validationPhaseFile,
 			errors: errs,
 		}
 	}
 
 	return allReadmeFiles, nil
 }
 
-func validateContributorRelativeUrls(contributors map[string]contributorProfileReadme) error {
-	// This function only validates relative avatar URLs for now, but it can be
-	// beefed up to validate more in the future.
+func validateContributorRelativeURLs(contributors map[string]contributorProfileReadme) error {
+	// This function only validates relative avatar URLs for now, but it can be beefed up to validate more in the future.
 	var errs []error
+
 	for _, con := range contributors {
+		// If the avatar URL is missing, we'll just assume that the Registry site build step will take care of filling
+		// in the data properly.
 		if con.frontmatter.AvatarURL == nil {
 			continue
 		}
 
-		isRelativeURL := strings.HasPrefix(*con.frontmatter.AvatarURL, ".") ||
-			strings.HasPrefix(*con.frontmatter.AvatarURL, "/")
-		if !isRelativeURL {
+		if !strings.HasPrefix(*con.frontmatter.AvatarURL, ".") || !strings.HasPrefix(*con.frontmatter.AvatarURL, "/") {
 			continue
 		}
 
@@ -291,10 +287,8 @@ func validateContributorRelativeUrls(contributors map[string]contributorProfileR
 			continue
 		}
 
-		absolutePath := strings.TrimSuffix(con.filePath, "README.md") +
-			*con.frontmatter.AvatarURL
-		_, err := os.Stat(absolutePath)
-		if err != nil {
+		absolutePath := strings.TrimSuffix(con.filePath, "README.md") + *con.frontmatter.AvatarURL
+		if _, err := os.ReadFile(absolutePath); err != nil {
 			errs = append(errs, xerrors.Errorf("%q: relative avatar path %q does not point to image in file system", con.filePath, absolutePath))
 		}
 	}
@@ -303,7 +297,7 @@ func validateContributorRelativeUrls(contributors map[string]contributorProfileR
 		return nil
 	}
 	return validationPhaseError{
-		phase:  validationPhaseAssetCrossReference,
+		phase:  validationPhaseCrossReference,
 		errors: errs,
 	}
 }
@@ -314,19 +308,18 @@ func validateAllContributorFiles() error {
 		return err
 	}
 
-	log.Printf("Processing %d README files\n", len(allReadmeFiles))
+	logger.Info(context.Background(), "processing README files", "num_files", len(allReadmeFiles))
 	contributors, err := parseContributorFiles(allReadmeFiles)
 	if err != nil {
 		return err
 	}
-	log.Printf("Processed %d README files as valid contributor profiles", len(contributors))
+	logger.Info(context.Background(), "processed README files as valid contributor profiles", "num_contributors", len(contributors))
 
-	err = validateContributorRelativeUrls(contributors)
-	if err != nil {
+	if err := validateContributorRelativeURLs(contributors); err != nil {
 		return err
 	}
-	log.Println("All relative URLs for READMEs are valid")
+	logger.Info(context.Background(), "all relative URLs for READMEs are valid")
 
-	log.Printf("Processed all READMEs in the %q directory\n", rootRegistryPath)
+	logger.Info(context.Background(), "processed all READMEs in directory", "dir", rootRegistryPath)
 	return nil
 }