chore: add validation for module source URLs #406

matifali · 2025-09-01T13:21:53Z

Would prevents issues like the one I fixed in #404

This change should have caught that error as

2025-09-01 13:12:45.132 [erro]  ...
    msg= Error during "README parsing" phase of README validation:
         - "registry/umair/modules/digitalocean-region/README.md": incorrect source URL format: found "registry.coder.com/coder/digitalocean-region/coder", expected "registry.coder.com/umair/digitalocean-region/coder"
error: script "validate-readme" exited with code 1

diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 53b912b..eb3cf8b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -63,8 +63,8 @@ jobs: - name: Set up Go uses: actions/setup-go@v5 with: - go-version: "1.23.2" - - name: Validate contributors + go-version: "1.25.0" + - name: Validate Reademde run: go build ./cmd/readmevalidation && ./readmevalidation - name: Remove build file artifact run: rm ./readmevalidation Signed-off-by: Muhammad Atif Ali <[email protected]>

- Downgrade Go version in CI to 1.24 for consistency. - Fix naming and path issues in `readmevalidation` code. - Improve regex validation for module and namespace names. - Correct typos and improve comments for clarity.

matifali · 2025-09-01T15:03:47Z

.github/workflows/ci.yaml

@@ -63,8 +63,8 @@ jobs:
      - name: Set up Go
        uses: actions/setup-go@v5
        with:
-          go-version: "1.23.2"
-      - name: Validate contributors
+          go-version: "1.24"


I was having weird issues with running Golang CI locally, and had to match the Go version.

matifali · 2025-09-01T15:04:24Z

cmd/readmevalidation/codermodules_test.go

Tests generated by gemini CLI

matifali · 2025-09-01T15:05:47Z

cmd/readmevalidation/coderresources.go

@@ -25,7 +25,7 @@ var (
 	terraformVersionRe = regexp.MustCompile(`^\s*\bversion\s+=`)

 	// Matches the format "> [!INFO]". Deliberately using a broad pattern to catch formatting issues that can mess up
-	// the renderer for the Registry website
+	// the renderer for the Registry website.


All these . are added by running golangci-lint locally, and I don't know how I can remove them. The CI also passes so I assume its fine leaving them

Oh, yeah, I feel like there should be a way to enforce this during the CI step, but this was my fault for not linting before opening one of the recent PRs

matifali · 2025-09-01T15:06:14Z

cmd/readmevalidation/coderresources.go

+			errs = append(errs, xerrors.New("registry does not support nested GFM alerts"))
 			continue
 		}

 		leadingWhitespace := currentMatch[1]
 		if len(leadingWhitespace) != 1 {
-			errs = append(errs, errors.New("GFM alerts must have one space between the '>' and the start of the GFM brackets"))
+			errs = append(errs, xerrors.New("GFM alerts must have one space between the '>' and the start of the GFM brackets"))


These changes are also done by golangci-lint

Parkreiner

This is close. I think there's a couple of changes we can make the code a bit better, though

Parkreiner · 2025-09-03T21:21:00Z

cmd/readmevalidation/coderresources.go

@@ -25,7 +25,7 @@ var (
 	terraformVersionRe = regexp.MustCompile(`^\s*\bversion\s+=`)

 	// Matches the format "> [!INFO]". Deliberately using a broad pattern to catch formatting issues that can mess up
-	// the renderer for the Registry website
+	// the renderer for the Registry website.


Oh, yeah, I feel like there should be a way to enforce this during the CI step, but this was my fault for not linting before opening one of the recent PRs

Parkreiner · 2025-09-03T21:25:38Z

cmd/readmevalidation/codermodules.go

 	"strings"

 	"golang.org/x/xerrors"
 )

+var (
+	terraformSourceRe = regexp.MustCompile(`^\s*source\s*=\s*"([^"]+)"`)


Do you have a good idea for what source fields typically look like? I feel like this regex is too broad (the [^"]+ alone feels like it's too permissive – I don't we should be supporting any arbitrary character that isn't a double quote)

I'm seeing all of these on the Terraform docs site:
https://developer.hashicorp.com/terraform/language/modules/sources

I think I'd prefer to have more explicit patterns for each source type example listed there

Oh, also, could we add a comment saying what general pattern we expect this to match?

One last update: now that I've looked at the rest of this file, if we're always going to use registry.coder.com/ in the source, we could bake that into the regex pattern

Parkreiner · 2025-09-03T21:33:46Z

cmd/readmevalidation/codermodules.go

+	terraformSourceRe = regexp.MustCompile(`^\s*source\s*=\s*"([^"]+)"`)
+)
+
+func extractNamespaceAndModuleFromPath(filePath string) (namespace string, moduleName string, err error) {


Nit: I know the code works, but I feel like it's a little confusing have to differentiate between the all-lowercase filepath package, versus the camelcase filePath parameter. Could we rename the latter to just path?

But going further, I feel like this information will probably end up being useful for other validation logic in the future (for modules or templates). Could we update the logic so that:

The coderResourceReadme type gets updated to include namespace and resourceName fields

extractNamespaceAndModuleFromPath is deleted, and instead has its logic inlined inside of parseCoderResourceReadme, which makes sure to add those fields to the struct before returning the value in the success case

validateModuleSourceURL is updated to remove the filePath parameter, and it instead receives the resource name and namespace from the struct

?

Parkreiner · 2025-09-03T21:45:26Z

cmd/readmevalidation/codermodules.go

+	return namespace, moduleName, nil
+}
+
+func validateModuleSourceURL(body string, filePath string) []error {


I wish I knew more about Coder templates. Do templates have a source field (or anything equivalent) that might need to be validated, too?

Parkreiner · 2025-09-03T21:53:37Z

cmd/readmevalidation/codermodules.go

+		if strings.HasPrefix(nextLine, "```") {
+			if strings.HasPrefix(nextLine, "```tf") && firstTerraformBlock {
+				isInsideTerraform = true
+				firstTerraformBlock = false
+			} else if isInsideTerraform {
+				// End of first terraform block.
+				break
+			}
+			continue
+		}


I think this can be rewritten as:

if strings.HasPrefix(nextLine, "```") { if strings.HasPrefix(nextLine, "```tf") { isInsideTerraform = true continue } if isInsideTerraform { break } }

I don't see the point of the firstTerraformBlock variable. If we always break at the end of the first Terraform code block, we don't really need to track anything extra

The goal is to check all tf blocks.

Maybe I'm missing something obvious, then. What validation requirement(s) are we trying to fulfill?

I was under the impression that we wanted the first Terraform block to contain a source field, but that other sources could be valid for the other blocks. Are we trying to keep all sources fully "boxed in" for Coder templates?

Not saying that the current code is wrong, but I'm not fully sure what we're trying to protect against, so I just want to make sure I'm giving good feedback

Parkreiner · 2025-09-03T22:08:52Z

cmd/readmevalidation/codermodules_test.go

+		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") {


Redundant condition here, too

Parkreiner · 2025-09-03T22:10:41Z

cmd/readmevalidation/codermodules_test.go

+	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)
+		}
+	})


I'm not sure what this test is checking for. I know we previously had concerns around modules having underscores in the slug – is that trying to catch that?

Because if so, I feel like we need to actually have underscores in the source strings

Parkreiner · 2025-09-03T22:11:39Z

cmd/readmevalidation/codermodules_test.go

+	t.Run("Invalid file path format", func(t *testing.T) {
+		t.Parallel()
+
+		body := "# Test Module"


Just to introduce a little bit more control into the test, I feel like we should make sure the body is fully valid, though if we apply the changes I mentioned above, I don't know if we need this test anymore

Parkreiner · 2025-09-03T22:12:42Z

cmd/readmevalidation/codermodules_test.go

+	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)))
+}


Do we need this separate function when we have strings.Contains?

Parkreiner · 2025-09-03T22:13:42Z

cmd/readmevalidation/codermodules_test.go

+		if s[i:i+len(substr)] == substr {
+			return i
+		}


This technically works, but re-slicing the string on each iteration feels a bit janky

- 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]>

matifali requested a review from Parkreiner September 1, 2025 13:21

matifali self-assigned this Sep 1, 2025

Merge branch 'main' into atif/validate-readme-source

5b6d878

matifali force-pushed the atif/validate-readme-source branch 3 times, most recently from 98efa49 to 898e773 Compare September 1, 2025 14:50

Update README validation and Go version

3b6b1ba

- Downgrade Go version in CI to 1.24 for consistency. - Fix naming and path issues in `readmevalidation` code. - Improve regex validation for module and namespace names. - Correct typos and improve comments for clarity.

matifali force-pushed the atif/validate-readme-source branch from 898e773 to 3b6b1ba Compare September 1, 2025 14:53

Remove master branch from lint trigger

7e94395

matifali commented Sep 1, 2025

View reviewed changes

matifali changed the title ~~chore: Aadd validation for Terraform module source URLs~~ chore: add validation for Terraform module source URLs Sep 1, 2025

matifali changed the title ~~chore: add validation for Terraform module source URLs~~ chore: add validation for module source URLs Sep 1, 2025

matifali requested review from bcpeinhardt and 35C4n0r September 3, 2025 04:31

Merge branch 'main' into atif/validate-readme-source

5419676

Parkreiner reviewed Sep 3, 2025

View reviewed changes

matifali requested a review from Parkreiner September 4, 2025 14:23

chore: add validation for module source URLs #406

Are you sure you want to change the base?

chore: add validation for module source URLs #406

Uh oh!

Conversation

matifali commented Sep 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matifali commented Sep 1, 2025 •

edited

Loading

Parkreiner Sep 4, 2025 •

edited

Loading