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 <me@matifali.dev>

- 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

        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

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

cmd/readmevalidation/coderresources.go

Parkreiner

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

cmd/readmevalidation/coderresources.go

cmd/readmevalidation/codermodules.go

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?

no its not applicable to templates

cmd/readmevalidation/codermodules.go

cmd/readmevalidation/codermodules_test.go

- 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 <matifali@users.noreply.github.com>

Parkreiner

We're looking better than before, but I'm still a confused by where the code is at now, and whether the current logic is what we want

Parkreiner · 2025-09-15T15:23:09Z

cmd/readmevalidation/codermodules.go

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


Do we want to swap the [^/]+ patterns for something even more specific? Right now, they allow any non-slash character, but don't we want to make sure the URLs don't have certain characters like underscores?

cmd/readmevalidation/codermodules.go

Parkreiner · 2025-09-15T15:27:04Z

cmd/readmevalidation/codermodules_test.go

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

- Use more specific regex pattern [a-zA-Z0-9-]+ instead of [^/]+ for namespace/module names - Process all Terraform blocks instead of just the first one - Report correct source if found in any block, only report incorrect sources if no correct source exists - Add comprehensive test cases for multiple Terraform blocks Co-authored-by: matifali <10648092+matifali@users.noreply.github.com>

DevelopmentCats · 2025-10-01T22:04:34Z

@Parkreiner I just want to poke you on this, to see if there is anything I can help unblock here.

Parkreiner · 2025-10-06T13:40:04Z

@DevelopmentCats Sorry, I had just gotten back from vacation last week, so this got a bit lost in the shuffle. I'll do an extra pass, and let you know

DevelopmentCats · 2025-10-06T13:45:59Z

@DevelopmentCats Sorry, I had just gotten back from vacation last week, so this got a bit lost in the shuffle. I'll do an extra pass, and let you know

It's all good. I know things have been crazy busy 😃

Parkreiner

@DevelopmentCats I think I'm still wrapping my head around the code a little bit, but I feel pretty sure the validation logic is still off, since we're silently removing errors in some cases

Parkreiner · 2025-10-06T13:45:14Z

cmd/readmevalidation/codermodules.go

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


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

Parkreiner · 2025-10-06T13:51:44Z

cmd/readmevalidation/codermodules.go

+	expectedSource := registryDomain + "/" + rm.namespace + "/" + rm.resourceName + "/coder"
+
+	trimmed := strings.TrimSpace(rm.body)
+	foundCorrectSource := false


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:

Have incorrectSources as the main validation state

Instead of having the actualSource == expectedSource check, only have a actualSource != expectedSource check. If that triggers, push the incorrect source to the incorrectSources slice. If things match, we'll do nothing, and let the code fall through to the rest of the loop

Once the main loop is done and we're done processing the lines, check if the slice is not empty. If it's not, return an error with a list of all sources that are incorrect

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 module "blah", would it be expected that the source URL should always have "blah" in it, too? If so, I think we'd need to check the name of each module block, and compare that against the source URL line

but if a module has a name like module "blah", would it be expected that the source URL should always have "blah" in it, too?

No it can be different. We just use the same name for convenience.

Parkreiner · 2025-10-06T13:58:07Z

cmd/readmevalidation/codermodules_test.go

 	})
 }
+
+func TestValidateModuleSourceURL(t *testing.T) {


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

Parkreiner · 2025-10-06T14:06:01Z

cmd/readmevalidation/codermodules_test.go

+var (
+	validModuleBody = `# Test Module
+
+` + "```tf\n" + `module "test-module" {


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 + signs are a bit hard to read in the GitHub UI, and this is a case where all the spacing and lines do matter

Parkreiner · 2025-10-06T14:08:13Z

cmd/readmevalidation/codermodules.go

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


I'm not sure why we wouldn't want to report all incorrect sources?

Parkreiner · 2025-10-06T14:10:56Z

cmd/readmevalidation/codermodules.go

+		return []error{xerrors.Errorf("invalid module path format: %s", rm.filePath)}
+	}
+
+	expectedSource := registryDomain + "/" + rm.namespace + "/" + rm.resourceName + "/coder"


Nit: since this variable isn't used for a while, we can move it down, closer to where it's used

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

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

9cc3d5a

matifali removed the request for review from 35C4n0r September 13, 2025 03:13

Parkreiner reviewed Sep 15, 2025

View reviewed changes

matifali requested a review from Parkreiner September 19, 2025 06:55

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

494e4de

matifali mentioned this pull request Oct 6, 2025

Formalize documentation requirements for modules. #56

Closed

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

d8c9ad1

Parkreiner reviewed Oct 6, 2025

View reviewed changes

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

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!

Uh oh!

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DevelopmentCats commented Oct 1, 2025

Uh oh!

Parkreiner commented Oct 6, 2025

Uh oh!

DevelopmentCats commented Oct 6, 2025

Uh oh!

Parkreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Parkreiner Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matifali commented Sep 1, 2025 •

edited

Loading

Parkreiner Oct 6, 2025 •

edited

Loading