- Notifications
You must be signed in to change notification settings - Fork72
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 ourterms of service andprivacy 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 from1 commit
72d7ee45b6d8783b6b1ba7e9439554196769c00c579cc3d5a343a2f2494e4ded8c9ad1File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
- 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.ContainsCo-authored-by: matifali <matifali@users.noreply.github.com>
- Loading branch information
Uh oh!
There was an error while loading.Please reload this page.
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -3,71 +3,64 @@ 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 + `/([^/]+)/([^/]+)/coder"`) | ||
| ||
| ) | ||
| func validateModuleSourceURL(rm coderResourceReadme) []error { | ||
| var errs []error | ||
| // Skip validation if we couldn't parse namespace/resourceName from path | ||
| ifrm.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 | ||
Member
| ||
| 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 | ||
| } | ||
| if isInsideTerraform { | ||
| break | ||
| } | ||
| continue | ||
| } | ||
| 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) { | ||
| 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 | ||
| } | ||
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,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" { | ||
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" | ||
| ) | ||
| 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() | ||
| 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() | ||
| 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()) | ||
| } | ||
| }) | ||
| } | ||