-
Notifications
You must be signed in to change notification settings - Fork 71
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 all 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,84 @@ package main | |
import ( | ||
"bufio" | ||
"context" | ||
"regexp" | ||
"strings" | ||
|
||
"golang.org/x/xerrors" | ||
) | ||
|
||
var ( | ||
// 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 + `/([a-zA-Z0-9-]+)/([a-zA-Z0-9-]+)/coder"`) | ||
) | ||
|
||
func validateModuleSourceURL(rm coderResourceReadme) []error { | ||
var errs []error | ||
|
||
// 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 := registryDomain + "/" + rm.namespace + "/" + rm.resourceName + "/coder" | ||
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. Nit: since this variable isn't used for a while, we can move it down, closer to where it's used |
||
|
||
trimmed := strings.TrimSpace(rm.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. |
||
var incorrectSources []string | ||
isInsideTerraform := false | ||
|
||
lineScanner := bufio.NewScanner(strings.NewReader(trimmed)) | ||
for lineScanner.Scan() { | ||
nextLine := lineScanner.Text() | ||
|
||
if strings.HasPrefix(nextLine, "```") { | ||
if strings.HasPrefix(nextLine, "```tf") { | ||
isInsideTerraform = true | ||
continue | ||
} | ||
Comment on lines
+38
to
+41
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. This is an edge case, but I feel like we want to handle cases where someone accidentally nests Terraform snippets inside each other like this: ```tf
```tf |
||
if isInsideTerraform { | ||
// End of current terraform block, continue to look for more | ||
isInsideTerraform = false | ||
} | ||
continue | ||
} | ||
|
||
if !isInsideTerraform { | ||
continue | ||
} | ||
|
||
// Check for source line in terraform blocks | ||
if matches := terraformSourceRe.FindStringSubmatch(nextLine); matches != nil { | ||
actualNamespace := matches[1] | ||
actualModule := matches[2] | ||
actualSource := registryDomain + "/" + actualNamespace + "/" + actualModule + "/coder" | ||
|
||
if actualSource == expectedSource { | ||
foundCorrectSource = true | ||
} else { | ||
// Collect incorrect sources but don't return immediately | ||
incorrectSources = append(incorrectSources, actualSource) | ||
} | ||
} | ||
} | ||
|
||
// If we found the correct source, ignore any incorrect ones | ||
if foundCorrectSource { | ||
return nil | ||
} | ||
|
||
// If we found incorrect sources but no correct one, report the first incorrect source | ||
if len(incorrectSources) > 0 { | ||
errs = append(errs, xerrors.Errorf("incorrect source URL format: found %q, expected %q", incorrectSources[0], expectedSource)) | ||
return errs | ||
} | ||
Comment on lines
+73
to
+77
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'm not sure why we wouldn't want to report all incorrect sources? |
||
|
||
// If we found no sources at all | ||
errs = append(errs, xerrors.Errorf("did not find correct source URL %q in any Terraform code block", expectedSource)) | ||
return errs | ||
} | ||
|
||
func validateCoderModuleReadmeBody(body string) []error { | ||
var errs []error | ||
|
||
|
@@ -94,6 +167,9 @@ func validateCoderModuleReadme(rm coderResourceReadme) []error { | |
for _, err := range validateCoderModuleReadmeBody(rm.body) { | ||
errs = append(errs, addFilePathToError(rm.filePath, err)) | ||
} | ||
for _, err := range validateModuleSourceURL(rm) { | ||
errs = append(errs, addFilePathToError(rm.filePath, err)) | ||
} | ||
for _, err := range validateResourceGfmAlerts(rm.body) { | ||
errs = append(errs, addFilePathToError(rm.filePath, err)) | ||
} | ||
|
@@ -143,4 +219,4 @@ func validateAllCoderModules() error { | |
} | ||
logger.Info(context.Background(), "all relative URLs for READMEs are valid", "resource_type", resourceType) | ||
return nil | ||
} | ||
} |
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 |
---|---|---|
|
@@ -2,12 +2,70 @@ 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" { | ||
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. Nit: I'm just now realizing how the sample MD contents are formatted, and I think I'd prefer for each one to be a single raw string. All the |
||
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" | ||
|
||
multipleBlocksValidBody = `# Test Module | ||
|
||
` + "```tf\n" + `module "other-module" { | ||
source = "registry.coder.com/other/module/coder" | ||
version = "1.0.0" | ||
} | ||
` + "```\n" + ` | ||
` + "```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" | ||
|
||
multipleBlocksInvalidBody = `# Test Module | ||
|
||
` + "```tf\n" + `module "test-module" { | ||
source = "registry.coder.com/wrong-namespace/test-module/coder" | ||
version = "1.0.0" | ||
} | ||
` + "```\n" + ` | ||
` + "```tf\n" + `module "other-module" { | ||
source = "registry.coder.com/another-wrong/test-module/coder" | ||
version = "1.0.0" | ||
agent_id = coder_agent.example.id | ||
} | ||
` + "```\n" | ||
) | ||
|
||
func TestValidateCoderResourceReadmeBody(t *testing.T) { | ||
t.Parallel() | ||
|
||
|
@@ -20,3 +78,115 @@ 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() | ||
|
||
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) | ||
} | ||
}) | ||
|
||
t.Run("Invalid source URL format - wrong namespace", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
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 !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() | ||
|
||
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 !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("Invalid file path format", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
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 !strings.Contains(errs[0].Error(), "invalid module path format") { | ||
t.Errorf("Expected path format error, got: %s", errs[0].Error()) | ||
} | ||
}) | ||
|
||
t.Run("Multiple blocks with valid source in second block", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
rm := coderResourceReadme{ | ||
resourceType: "modules", | ||
filePath: "registry/test-namespace/modules/test-module/README.md", | ||
namespace: "test-namespace", | ||
resourceName: "test-module", | ||
body: multipleBlocksValidBody, | ||
} | ||
errs := validateModuleSourceURL(rm) | ||
if len(errs) != 0 { | ||
t.Errorf("Expected no errors, got: %v", errs) | ||
} | ||
}) | ||
|
||
t.Run("Multiple blocks with incorrect source in second block", func(t *testing.T) { | ||
t.Parallel() | ||
|
||
rm := coderResourceReadme{ | ||
resourceType: "modules", | ||
filePath: "registry/test-namespace/modules/test-module/README.md", | ||
namespace: "test-namespace", | ||
resourceName: "test-module", | ||
body: multipleBlocksInvalidBody, | ||
} | ||
errs := validateModuleSourceURL(rm) | ||
if len(errs) != 1 { | ||
t.Errorf("Expected 1 error, got %d: %v", len(errs), errs) | ||
} | ||
if !strings.Contains(errs[0].Error(), "incorrect source URL format") { | ||
t.Errorf("Expected source URL format error, got: %s", errs[0].Error()) | ||
} | ||
}) | ||
} |
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.