Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member Author

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.

- name: Validate README
run: go build ./cmd/readmevalidation && ./readmevalidation
- name: Remove build file artifact
run: rm ./readmevalidation
1 change: 0 additions & 1 deletion .github/workflows/golangci-lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ on:
push:
branches:
- main
- master
pull_request:

permissions:
Expand Down
73 changes: 73 additions & 0 deletions cmd/readmevalidation/codermodules.go
Original file line number Diff line number Diff line change
Expand Up @@ -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*"([^"]+)"`)
)

func extractNamespaceAndModuleFromPath(filePath string) (namespace string, moduleName string, err error) {
// 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no its not applicable to templates

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
Copy link
Member

@Parkreiner Parkreiner Oct 6, 2025

Choose a reason for hiding this comment

The 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:

  1. Have incorrectSources as the main validation state
  2. 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
  3. 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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
}

if isInsideTerraform {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reduce the nesting if we do this:

if !isInsideTerraform {
  continue
}

// 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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the break above, we can turn this else if into a standalone if

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But also, do we want to extract the "registry.coder.com/" into a separate global const variable? We're already using it here and to create the expected source, then I feel like it'd be handy to avoid any accidental typos

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

Expand Down Expand Up @@ -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))
}
Expand Down
83 changes: 83 additions & 0 deletions cmd/readmevalidation/codermodules_test.go
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests generated by gemini CLI

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Up @@ -20,3 +20,86 @@ func TestValidateCoderResourceReadmeBody(t *testing.T) {
}
})
}

func TestValidateModuleSourceURL(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The 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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I feel like reading through these bodies is going to be a pain if the validation logic ever breaks, and we need to debug things. A couple of things come to mind:

  1. I feel like we could have at least one of the tests reference the sampleReadmeBody.md file that's currently being embedded
  2. For all the others, I don't know if a separate file makes sense, but we could put these into raw strings, and extract them outside the functions so they're a little easier to read

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") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This len(errs) > 0 is redundant

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

t.Run("Invalid file path format", func(t *testing.T) {
t.Parallel()

body := "# Test Module"
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)))
}

func indexOfSubstring(s, substr string) int {
for i := 0; i <= len(s)-len(substr); i++ {
if s[i:i+len(substr)] == substr {
return i
}
}
return -1
}
12 changes: 6 additions & 6 deletions cmd/readmevalidation/coderresources.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
gfmAlertRegex = regexp.MustCompile(`^>(\s*)\[!(\w+)\](\s*)(.*)`)
)

Expand All @@ -39,7 +39,7 @@ type coderResourceFrontmatter struct {
}

// A slice version of the struct tags from coderResourceFrontmatter. Might be worth using reflection to generate this
// list at runtime in the future, but this should be okay for now
// list at runtime in the future, but this should be okay for now.
var supportedCoderResourceStructKeys = []string{
"description", "icon", "display_name", "verified", "tags", "supported_os",
// TODO: This is an old, officially deprecated key from the archived coder/modules repo. We can remove this once we
Expand Down Expand Up @@ -315,15 +315,15 @@ func validateResourceGfmAlerts(readmeBody string) []error {
}

// Nested GFM alerts is such a weird mistake that it's probably not really safe to keep trying to process the
// rest of the content, so this will prevent any other validations from happening for the given line
// rest of the content, so this will prevent any other validations from happening for the given line.
if isInsideGfmQuotes {
errs = append(errs, errors.New("registry does not support nested GFM alerts"))
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"))
}
isInsideGfmQuotes = true

Expand All @@ -347,7 +347,7 @@ func validateResourceGfmAlerts(readmeBody string) []error {
}
}

if gfmAlertRegex.Match([]byte(sourceLine)) {
if gfmAlertRegex.MatchString(sourceLine) {
errs = append(errs, xerrors.Errorf("README has an incomplete GFM alert at the end of the file"))
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/readmevalidation/contributors.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ type contributorProfileFrontmatter struct {
}

// A slice version of the struct tags from contributorProfileFrontmatter. Might be worth using reflection to generate
// this list at runtime in the future, but this should be okay for now
// this list at runtime in the future, but this should be okay for now.
var supportedContributorProfileStructKeys = []string{"display_name", "bio", "status", "avatar", "linkedin", "github", "website", "support_email"}

type contributorProfileReadme struct {
Expand Down
11 changes: 5 additions & 6 deletions cmd/readmevalidation/repostructure.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,11 @@ import (

var supportedUserNameSpaceDirectories = append(supportedResourceTypes, ".images")

// validNameRe validates that names contain only alphanumeric characters and hyphens
// validNameRe validates that names contain only alphanumeric characters and hyphens.
var validNameRe = regexp.MustCompile(`^[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?$`)


// validateCoderResourceSubdirectory validates that the structure of a module or template within a namespace follows all
// expected file conventions
// expected file conventions.
func validateCoderResourceSubdirectory(dirPath string) []error {
resourceDir, err := os.Stat(dirPath)
if err != nil {
Expand Down Expand Up @@ -47,7 +46,7 @@ func validateCoderResourceSubdirectory(dirPath string) []error {
continue
}

// Validate module/template name
// Validate module/template name.
if !validNameRe.MatchString(f.Name()) {
errs = append(errs, xerrors.Errorf("%q: name contains invalid characters (only alphanumeric characters and hyphens are allowed)", path.Join(dirPath, f.Name())))
continue
Expand Down Expand Up @@ -90,7 +89,7 @@ func validateRegistryDirectory() []error {
continue
}

// Validate namespace name
// Validate namespace name.
if !validNameRe.MatchString(nDir.Name()) {
allErrs = append(allErrs, xerrors.Errorf("%q: namespace name contains invalid characters (only alphanumeric characters and hyphens are allowed)", namespacePath))
continue
Expand Down Expand Up @@ -136,7 +135,7 @@ func validateRegistryDirectory() []error {

// validateRepoStructure validates that the structure of the repo is "correct enough" to do all necessary validation
// checks. It is NOT an exhaustive validation of the entire repo structure – it only checks the parts of the repo that
// are relevant for the main validation steps
// are relevant for the main validation steps.
func validateRepoStructure() error {
var errs []error
if vrdErrs := validateRegistryDirectory(); len(vrdErrs) != 0 {
Expand Down
2 changes: 1 addition & 1 deletion go.mod
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
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
"fmt:ci": "bun x prettier --check . && terraform fmt -check -recursive -diff",
"terraform-validate": "./scripts/terraform_validate.sh",
"test": "./scripts/terraform_test_all.sh",
"update-version": "./update-version.sh"
"update-version": "./update-version.sh",
"validate-readme": "go build ./cmd/readmevalidation && ./readmevalidation"
},
"devDependencies": {
"@types/bun": "^1.2.21",
Expand Down