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*"([^"]+)"`)
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

)

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is to check all tf blocks.

Copy link
Member

@Parkreiner Parkreiner Sep 4, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to make sure all sources for all codeblock in a module README.md are in correct format. This should not apply to templates if it does its probably a bug.

Copy link
Member

@Parkreiner Parkreiner Sep 5, 2025

Choose a reason for hiding this comment

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

Sorry, I meant "modules" instead of "templates"

But if the goal is to check all Terraform blocks, then we'll need to remove the firstTerraformBlock variable completely.

With how the code is currently set up, once we hit the end of the first block, we always hit the else if block and immediately break out of the loop. We would need to update the loop logic to keep going after reaching the end of a block – flipping isInsideTerraform back to false and then probably just 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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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"))
Comment on lines +334 to +340
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes are also done by golangci-lint

}
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