Address PR review comments

- Make regex more specific for registry.coder.com patterns only
- Refactor to add namespace and resourceName fields to coderResourceReadme struct
- Inline path parsing logic into parseCoderResourceReadme
- Update validateModuleSourceURL to use struct fields instead of filePath parameter
- Simplify Terraform block detection logic
- Reduce nesting with early continue statements
- Add comment explaining regex pattern
- Extract registry.coder.com into a constant
- Improve test readability with extracted variables
- Remove redundant checks in tests
- Replace custom contains function with strings.Contains

Co-authored-by: matifali <[email protected]>

Author: blink-so[bot]

cmd/readmevalidation/codermodules.go

@@ -3,71 +3,64 @@ package main
 import (
 	"bufio"
 	"context"
-	"path/filepath"
 	"regexp"
 	"strings"
 
 	"golang.org/x/xerrors"
 )
 
 var (
-	terraformSourceRe = regexp.MustCompile(`^\s*source\s*=\s*"([^"]+)"`)
+	// Matches Terraform source lines with registry.coder.com URLs
+	// Pattern: source = "registry.coder.com/namespace/module/coder"
+	terraformSourceRe = regexp.MustCompile(`^\s*source\s*=\s*"` + registryDomain + `/([^/]+)/([^/]+)/coder"`)
 )
 
-func extractNamespaceAndModuleFromPath(filePath string) (namespace string, moduleName string, err error) {
-	// Expected path format: registry/<namespace>/modules/<module-name>/README.md.
-	parts := strings.Split(filepath.Clean(filePath), string(filepath.Separator))
-	if len(parts) < 5 || parts[0] != "registry" || parts[2] != "modules" || parts[4] != "README.md" {
-		return "", "", xerrors.Errorf("invalid module path format: %s", filePath)
-	}
-	namespace = parts[1]
-	moduleName = parts[3]
-	return namespace, moduleName, nil
-}
-
-func validateModuleSourceURL(body string, filePath string) []error {
+func validateModuleSourceURL(rm coderResourceReadme) []error {
 	var errs []error
 
-	namespace, moduleName, err := extractNamespaceAndModuleFromPath(filePath)
-	if err != nil {
-		return []error{err}
+	// Skip validation if we couldn't parse namespace/resourceName from path
+	if rm.namespace == "" || rm.resourceName == "" {
+		return []error{xerrors.Errorf("invalid module path format: %s", rm.filePath)}
 	}
 
-	expectedSource := "registry.coder.com/" + namespace + "/" + moduleName + "/coder"
+	expectedSource := registryDomain + "/" + rm.namespace + "/" + rm.resourceName + "/coder"
 
-	trimmed := strings.TrimSpace(body)
+	trimmed := strings.TrimSpace(rm.body)
 	foundCorrectSource := false
 	isInsideTerraform := false
-	firstTerraformBlock := true
 
 	lineScanner := bufio.NewScanner(strings.NewReader(trimmed))
 	for lineScanner.Scan() {
 		nextLine := lineScanner.Text()
 
 		if strings.HasPrefix(nextLine, "```") {
-			if strings.HasPrefix(nextLine, "```tf") && firstTerraformBlock {
+			if strings.HasPrefix(nextLine, "```tf") {
 				isInsideTerraform = true
-				firstTerraformBlock = false
-			} else if isInsideTerraform {
-				// End of first terraform block.
+				continue
+			}
+			if isInsideTerraform {
 				break
 			}
 			continue
 		}
 
-		if isInsideTerraform {
-			// Check for any source line in the first terraform block.
-			if matches := terraformSourceRe.FindStringSubmatch(nextLine); matches != nil {
-				actualSource := matches[1]
-				if actualSource == expectedSource {
-					foundCorrectSource = true
-					break
-				} else if strings.HasPrefix(actualSource, "registry.coder.com/") && strings.Contains(actualSource, "/"+moduleName+"/coder") {
-					// Found source for this module but with wrong namespace/format.
-					errs = append(errs, xerrors.Errorf("incorrect source URL format: found %q, expected %q", actualSource, expectedSource))
-					return errs
-				}
+		if !isInsideTerraform {
+			continue
+		}
+
+		// Check for source line in the first terraform block
+		if matches := terraformSourceRe.FindStringSubmatch(nextLine); matches != nil {
+			actualNamespace := matches[1]
+			actualModule := matches[2]
+			actualSource := registryDomain + "/" + actualNamespace + "/" + actualModule + "/coder"
+			
+			if actualSource == expectedSource {
+				foundCorrectSource = true
+				break
 			}
+			// Found a registry.coder.com source but with wrong namespace/module
+			errs = append(errs, xerrors.Errorf("incorrect source URL format: found %q, expected %q", actualSource, expectedSource))
+			return errs
 		}
 	}
 
@@ -164,7 +157,7 @@ func validateCoderModuleReadme(rm coderResourceReadme) []error {
 	for _, err := range validateCoderModuleReadmeBody(rm.body) {
 		errs = append(errs, addFilePathToError(rm.filePath, err))
 	}
-	for _, err := range validateModuleSourceURL(rm.body, rm.filePath) {
+	for _, err := range validateModuleSourceURL(rm) {
 		errs = append(errs, addFilePathToError(rm.filePath, err))
 	}
 	for _, err := range validateResourceGfmAlerts(rm.body) {
@@ -216,4 +209,4 @@ func validateAllCoderModules() error {
 	}
 	logger.Info(context.Background(), "all relative URLs for READMEs are valid", "resource_type", resourceType)
 	return nil
-}
+}

cmd/readmevalidation/codermodules_test.go

@@ -2,12 +2,42 @@ package main
 
 import (
 	_ "embed"
+	"strings"
 	"testing"
 )
 
 //go:embed testSamples/sampleReadmeBody.md
 var testBody string
 
+// Test bodies extracted for better readability
+var (
+	validModuleBody = `# Test Module
+
+` + "```tf\n" + `module "test-module" {
+  source   = "registry.coder.com/test-namespace/test-module/coder"
+  version  = "1.0.0"
+  agent_id = coder_agent.example.id
+}
+` + "```\n"
+
+	wrongNamespaceBody = `# Test Module
+
+` + "```tf\n" + `module "test-module" {
+  source   = "registry.coder.com/wrong-namespace/test-module/coder"
+  version  = "1.0.0"
+  agent_id = coder_agent.example.id
+}
+` + "```\n"
+
+	missingSourceBody = `# Test Module
+
+` + "```tf\n" + `module "test-module" {
+  version  = "1.0.0"
+  agent_id = coder_agent.example.id
+}
+` + "```\n"
+)
+
 func TestValidateCoderResourceReadmeBody(t *testing.T) {
 	t.Parallel()
 
@@ -27,9 +57,14 @@ func TestValidateModuleSourceURL(t *testing.T) {
 	t.Run("Valid source URL format", func(t *testing.T) {
 		t.Parallel()
 
-		body := "# Test Module\n\n```tf\nmodule \"test-module\" {\n  source   = \"registry.coder.com/test-namespace/test-module/coder\"\n  version  = \"1.0.0\"\n  agent_id = coder_agent.example.id\n}\n```\n"
-		filePath := "registry/test-namespace/modules/test-module/README.md"
-		errs := validateModuleSourceURL(body, filePath)
+		rm := coderResourceReadme{
+			resourceType: "modules",
+			filePath:     "registry/test-namespace/modules/test-module/README.md",
+			namespace:    "test-namespace",
+			resourceName: "test-module",
+			body:         validModuleBody,
+		}
+		errs := validateModuleSourceURL(rm)
 		if len(errs) != 0 {
 			t.Errorf("Expected no errors, got: %v", errs)
 		}
@@ -38,68 +73,57 @@ func TestValidateModuleSourceURL(t *testing.T) {
 	t.Run("Invalid source URL format - wrong namespace", func(t *testing.T) {
 		t.Parallel()
 
-		body := "# Test Module\n\n```tf\nmodule \"test-module\" {\n  source   = \"registry.coder.com/wrong-namespace/test-module/coder\"\n  version  = \"1.0.0\"\n  agent_id = coder_agent.example.id\n}\n```\n"
-		filePath := "registry/test-namespace/modules/test-module/README.md"
-		errs := validateModuleSourceURL(body, filePath)
+		rm := coderResourceReadme{
+			resourceType: "modules",
+			filePath:     "registry/test-namespace/modules/test-module/README.md",
+			namespace:    "test-namespace",
+			resourceName: "test-module",
+			body:         wrongNamespaceBody,
+		}
+		errs := validateModuleSourceURL(rm)
 		if len(errs) != 1 {
 			t.Errorf("Expected 1 error, got %d: %v", len(errs), errs)
 		}
-		if len(errs) > 0 && !contains(errs[0].Error(), "incorrect source URL format") {
+		if !strings.Contains(errs[0].Error(), "incorrect source URL format") {
 			t.Errorf("Expected source URL format error, got: %s", errs[0].Error())
 		}
 	})
 
 	t.Run("Missing source URL", func(t *testing.T) {
 		t.Parallel()
 
-		body := "# Test Module\n\n```tf\nmodule \"other-module\" {\n  source   = \"registry.coder.com/other/other-module/coder\"\n  version  = \"1.0.0\"\n  agent_id = coder_agent.example.id\n}\n```\n"
-		filePath := "registry/test-namespace/modules/test-module/README.md"
-		errs := validateModuleSourceURL(body, filePath)
+		rm := coderResourceReadme{
+			resourceType: "modules",
+			filePath:     "registry/test-namespace/modules/test-module/README.md",
+			namespace:    "test-namespace",
+			resourceName: "test-module",
+			body:         missingSourceBody,
+		}
+		errs := validateModuleSourceURL(rm)
 		if len(errs) != 1 {
 			t.Errorf("Expected 1 error, got %d: %v", len(errs), errs)
 		}
-		if len(errs) > 0 && !contains(errs[0].Error(), "did not find correct source URL") {
+		if !strings.Contains(errs[0].Error(), "did not find correct source URL") {
 			t.Errorf("Expected missing source URL error, got: %s", errs[0].Error())
 		}
 	})
 
-	t.Run("Module name with hyphens vs underscores", func(t *testing.T) {
-		t.Parallel()
-
-		body := "# Test Module\n\n```tf\nmodule \"test_module\" {\n  source   = \"registry.coder.com/test-namespace/test-module/coder\"\n  version  = \"1.0.0\"\n  agent_id = coder_agent.example.id\n}\n```\n"
-		filePath := "registry/test-namespace/modules/test-module/README.md"
-		errs := validateModuleSourceURL(body, filePath)
-		if len(errs) != 0 {
-			t.Errorf("Expected no errors for hyphen/underscore variation, got: %v", errs)
-		}
-	})
-
 	t.Run("Invalid file path format", func(t *testing.T) {
 		t.Parallel()
 
-		body := "# Test Module"
-		filePath := "invalid/path/format"
-		errs := validateModuleSourceURL(body, filePath)
+		rm := coderResourceReadme{
+			resourceType: "modules",
+			filePath:     "invalid/path/format",
+			namespace:    "", // Empty because path parsing failed
+			resourceName: "", // Empty because path parsing failed
+			body:         "# Test Module",
+		}
+		errs := validateModuleSourceURL(rm)
 		if len(errs) != 1 {
 			t.Errorf("Expected 1 error, got %d: %v", len(errs), errs)
 		}
-		if len(errs) > 0 && !contains(errs[0].Error(), "invalid module path format") {
+		if !strings.Contains(errs[0].Error(), "invalid module path format") {
 			t.Errorf("Expected path format error, got: %s", errs[0].Error())
 		}
 	})
-}
-
-func contains(s, substr string) bool {
-	return len(s) >= len(substr) && (s == substr || (len(s) > len(substr) &&
-		(s[:len(substr)] == substr || s[len(s)-len(substr):] == substr ||
-			indexOfSubstring(s, substr) >= 0)))
-}
-
-func indexOfSubstring(s, substr string) int {
-	for i := 0; i <= len(s)-len(substr); i++ {
-		if s[i:i+len(substr)] == substr {
-			return i
-		}
-	}
-	return -1
-}
+}

cmd/readmevalidation/coderresources.go

@@ -18,6 +18,7 @@ var (
 	supportedResourceTypes = []string{"modules", "templates"}
 	operatingSystems       = []string{"windows", "macos", "linux"}
 	gfmAlertTypes          = []string{"NOTE", "IMPORTANT", "CAUTION", "WARNING", "TIP"}
+	registryDomain         = "registry.coder.com"
 
 	// 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
@@ -53,6 +54,8 @@ var supportedCoderResourceStructKeys = []string{
 type coderResourceReadme struct {
 	resourceType string
 	filePath     string
+	namespace    string
+	resourceName string
 	body         string
 	frontmatter  coderResourceFrontmatter
 }
@@ -183,9 +186,20 @@ func parseCoderResourceReadme(resourceType string, rm readme) (coderResourceRead
 		return coderResourceReadme{}, []error{xerrors.Errorf("%q: failed to parse: %v", rm.filePath, err)}
 	}
 
+	// Extract namespace and resource name from file path
+	// Expected path format: registry/<namespace>/<resourceType>/<resource-name>/README.md
+	var namespace, resourceName string
+	parts := strings.Split(path.Clean(rm.filePath), "/")
+	if len(parts) >= 5 && parts[0] == "registry" && parts[2] == resourceType && parts[4] == "README.md" {
+		namespace = parts[1]
+		resourceName = parts[3]
+	}
+
 	return coderResourceReadme{
 		resourceType: resourceType,
 		filePath:     rm.filePath,
+		namespace:    namespace,
+		resourceName: resourceName,
 		body:         body,
 		frontmatter:  yml,
 	}, nil