-
Notifications
You must be signed in to change notification settings - Fork 73
chore: add validation for module source URLs #406
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.
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
base: main
Are you sure you want to change the base?
Changes from 4 commits
72d7ee4
5b6d878
3b6b1ba
7e94395
5419676
9c00c57
9cc3d5a
343a2f2
494e4de
d8c9ad1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,6 @@ on: | |
push: | ||
branches: | ||
- main | ||
- master | ||
pull_request: | ||
|
||
permissions: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,11 +3,81 @@ package main | |
import ( | ||
"bufio" | ||
"context" | ||
"path/filepath" | ||
"regexp" | ||
"strings" | ||
|
||
"golang.org/x/xerrors" | ||
) | ||
|
||
var ( | ||
terraformSourceRe = regexp.MustCompile(`^\s*source\s*=\s*"([^"]+)"`) | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
) | ||
|
||
func extractNamespaceAndModuleFromPath(filePath string) (namespace string, moduleName string, err error) { | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
// 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 { | ||
|
||
var errs []error | ||
|
||
namespace, moduleName, err := extractNamespaceAndModuleFromPath(filePath) | ||
if err != nil { | ||
return []error{err} | ||
} | ||
|
||
expectedSource := "registry.coder.com/" + namespace + "/" + moduleName + "/coder" | ||
|
||
trimmed := strings.TrimSpace(body) | ||
foundCorrectSource := false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edit: I think there's something we can salvage from this comment, but I realized in the second comment in the chain that it doesn't do everything we want I don't know if this variable makes sense. With how it's set up right now, if we have at least one correct source in the README file, we'll automatically ignore all other incorrect sources. And from a state modeling perspective, the variable also feels redundant, when a single slice should be able to give us all the info we need What makes more sense to me is to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, now that I'm thinking over the code more, I think I understand what the old code was trying to do, and why my suggestion doesn't actually work I'm not super up to speed on our Terraform conventions, but if a module has a name like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No it can be different. We just use the same name for convenience. |
||
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 { | ||
isInsideTerraform = true | ||
firstTerraformBlock = false | ||
} else if isInsideTerraform { | ||
// End of first terraform block. | ||
break | ||
} | ||
continue | ||
} | ||
Parkreiner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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 !foundCorrectSource { | ||
errs = append(errs, xerrors.Errorf("did not find correct source URL %q in first Terraform code block", expectedSource)) | ||
} | ||
|
||
return errs | ||
} | ||
|
||
func validateCoderModuleReadmeBody(body string) []error { | ||
var errs []error | ||
|
||
|
@@ -94,6 +164,9 @@ 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) { | ||
errs = append(errs, addFilePathToError(rm.filePath, err)) | ||
} | ||
for _, err := range validateResourceGfmAlerts(rm.body) { | ||
errs = append(errs, addFilePathToError(rm.filePath, err)) | ||
} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tests generated by gemini CLI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assuming we still want to process all the blocks in a file, I think it'd be really good to have some test cases that include 2+ blocks |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,3 +20,86 @@ func TestValidateCoderResourceReadmeBody(t *testing.T) { | |
} | ||
}) | ||
} | ||
|
||
func TestValidateModuleSourceURL(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the test setup is pretty good. Just to account for the other comment I added, though, I feel like we should have one more test that validates what happens if you have a block with an incorrect body, and 2+ other blocks with correct bodies |
||
t.Parallel() | ||
|
||
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) | ||
if len(errs) != 0 { | ||
t.Errorf("Expected no errors, got: %v", errs) | ||
} | ||
}) | ||
|
||
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) | ||
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") { | ||
|
||
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) | ||
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") { | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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) | ||
} | ||
}) | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
t.Run("Invalid file path format", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
body := "# Test Module" | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
filePath := "invalid/path/format" | ||
errs := validateModuleSourceURL(body, filePath) | ||
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") { | ||
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))) | ||
} | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
func indexOfSubstring(s, substr string) int { | ||
for i := 0; i <= len(s)-len(substr); i++ { | ||
if s[i:i+len(substr)] == substr { | ||
return i | ||
} | ||
matifali marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
return -1 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
module coder.com/coder-registry | ||
|
||
go 1.23.2 | ||
go 1.24 | ||
|
||
require ( | ||
cdr.dev/slog v1.6.1 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was having weird issues with running Golang CI locally, and had to match the Go version.