chore: add logic to validate all modules #11

Parkreiner · 2025-04-19T02:51:50Z

Changes made

Added functionality to validate the structure of module README files (frontmatter and the README body)
Added a really basic snapshot-ish test for the module README body validation
Updated README files that were previously violating README requirements (the old modules validation logic wasn't catching these)
Changed ValidationPhase from an int to a string

Notes

I've brought this up before, but I do think that it should be safe to remove the maintainer_github field, because that information can be intuited by the directory path. I didn't add any checks for it, because the new Registry site build process shouldn't need them
With how the current logic is set up (and assuming that templates and modules maintain the same README structure for a while), it'll be really easy to add
This setup works for now, but sometime down the line, I would like to refactor the code to use concurrency. Not for performance, but for "correctness" reasons – the structure of a concurrent program naturally lines up with the needs of a data pipeline. It'll make it easy to improve the granularity of the error messages, and help limit the amount of back-and-forth contributors will need to do with CI if they're editing multiple README files at once
The one test I added was to help make development easier for me, but I am planning to flesh that test suite out down the line

Parkreiner · 2025-04-21T12:36:42Z

cmd/readmevalidation/coderResources.go

+type coderResourceReadmes map[string]coderResourceReadme
+
+func (crr coderResourceReadmes) Get(filePath string) (coderResourceReadme, bool) {
+	rm, ok := crr[filePath]
+	return rm, ok
+}


Wasn't sure if this pattern made sense in some cases. The idea is that if you have a map, the type signature doesn't communicate how the keys are formatted. With a custom type, the method makes it clear that you need a filePath to index the value by

I think the type and method are unnecessary, you can leave a comment at the usage points informing the map structure since it doesn't travel far in the code when it is used.

Parkreiner · 2025-04-21T12:40:00Z

cmd/readmevalidation/coderResources.go

+	var errs []error
+	errs = append(errs, validateReadmeBody(trimmed)...)


Wasn't sure about whether Go has any rules of thumb for when to treat data as immutable, but I treated the results of validateReadme as if it were, even though there's no other owner for the slice after the function ends, and I could've just mutated the result directly

Parkreiner · 2025-04-21T12:41:11Z

cmd/readmevalidation/coderResources.go

+// would be a big improvement.
+var terraformVersionRe = regexp.MustCompile("^\\s*\\bversion\\s+=")
+
+func validateCoderResourceReadmeBody(body string) []error {


This function is convoluted, but at least with it seeming to work (from all the input files that exist right now), I'd rather get unit tests in place for it, and then use those to guide refactoring the function to do AST parsing

Parkreiner · 2025-04-21T12:45:51Z

cmd/readmevalidation/coderResources_test.go

Didn't want to spend too much time on tests for this iteration, but needed to isolate the README-parsing logic to figure out how to make sure it worked faster

Parkreiner · 2025-04-21T12:46:28Z

cmd/readmevalidation/readmeFiles.go

+// Todo: This seems to work okay for now, but the really proper way of doing
+// this is by parsing this as an AST, and then checking the resulting nodes
+func validateReadmeBody(body string) []error {


Would also like to get tests in place, and then use those to refactor this

chore: update README files and validation to reflect new requirements

bcpeinhardt

😎
We can talk about cleaning this and #10 up a bit and possibly moving it into coder/coder-registry post Profile launch.

f0ssel

Looks good, I'm fine without tests because the existing readmes act as one for now and we can circle back with other work.

f0ssel · 2025-04-30T16:21:05Z

cmd/readmevalidation/coderResources.go

+type coderResourceReadmes map[string]coderResourceReadme
+
+func (crr coderResourceReadmes) Get(filePath string) (coderResourceReadme, bool) {
+	rm, ok := crr[filePath]
+	return rm, ok
+}


I think the type and method are unnecessary, you can leave a comment at the usage points informing the map structure since it doesn't travel far in the code when it is used.

f0ssel · 2025-04-30T16:24:52Z

cmd/readmevalidation/coderResources.go

+func validateCoderResourceTags(tags []string) error {
+	if len(tags) == 0 {
+		return nil
+	}


tags can be nil here, right? Is that valid, or should we check for it? len will return 0 either way, but if you want to enforce that people put it in the frontmatter with an empty array we should fail on nil. If we are cool with omitting it there's nothing to do here, but we may want to put a comment saying that.

Oh, good call. I didn't think about it before, but I'll go ahead and add a check, even though I don't think it'll matter much in practice. The YAML library should guarantee that the tags field gets parsed as an allocated array, but since the function can still be called separately, there's a risk of edge cases down the line

Gotcha, so even if the key is completely ommited it still parses to a slice with length 0? If so they it's safe, but I wasn't sure.

It could be nil if the field were optional, but it's required. So that gives us some guarantees, because parser errors out on omitted keys, instead of initializing them with nil values. If a user forgets the key, CI should ding them

f0ssel · 2025-04-30T16:29:55Z

cmd/readmevalidation/contributors.go

@@ -27,7 +27,7 @@ type contributorProfileFrontmatter struct {
 	ContributorStatus *string `yaml:"status"`
 }

-type contributorProfile struct {
+type contributorProfileReadme struct {


f0ssel · 2025-04-30T16:30:17Z

cmd/readmevalidation/readmeFiles.go

-		strings.NewReader(strings.TrimSpace(readmeText)),
-	)
+
+	lineScanner := bufio.NewScanner(strings.NewReader(strings.TrimSpace(readmeText)))


f0ssel · 2025-04-30T16:33:24Z

Oh and let's get rid of the camelCase, we do all lowercase for all go files like repostructure.go over repoStructure.go. If needed we can drop to snake_casing but I prefer to try to have simpler filenames first.

Parkreiner added 17 commits April 18, 2025 19:16

refactor: update file structure to reflect new changes

404b1e0

refactor: start splitting up files

36d3389

refactor: more domain splitting

1a5c102

refactor: remove directory validation from contributors file

52c1f45

fix: update repo structure checks

812dd8f

fix: improve check for user namespace subdirectories

1dcc645

docs: add missing words to comment

eea1e05

docs: update typo

fdc0f98

refactor: make code easier to read

31c69e2

chore: add simplified version of module validation logic

1634c40

wip: get initial version of h1 validation done

05f58e3

wip: commit progress on module validation

4926849

fix: update base README logic to ignore code blocks

e350be4

fix: update README again

53c3efa

fix: remove line numbers in error messages

ba06233

fix: update README validation to block empty strings

db2fd0e

get initial version of parsing working

1d3c04f

Parkreiner self-assigned this Apr 19, 2025

Parkreiner commented Apr 21, 2025

View reviewed changes

Parkreiner mentioned this pull request Apr 22, 2025

fix: update repo structure validation logic to disallow false positives #10

Merged

Parkreiner requested a review from f0ssel April 22, 2025 16:31

Parkreiner added 5 commits April 28, 2025 19:22

fix: update README files

4564068

fix: remove employer field entirely

c731a5c

fix: make Github field optional

7e8d4b7

Merge pull request #18 from coder/mes/tighten-schema

d4d3074

chore: update README files and validation to reflect new requirements

Merge branch 'mes/module-validation' into mes/module-validation-2

2c18e0b

Parkreiner requested a review from bcpeinhardt April 28, 2025 20:30

bcpeinhardt approved these changes Apr 29, 2025

View reviewed changes

f0ssel approved these changes Apr 30, 2025

View reviewed changes

Parkreiner added 5 commits May 2, 2025 15:19

Merge branch 'main' into mes/module-validation

cc51af1

refactor: rename files

2e5f3bf

Merge branch 'mes/module-validation' into mes/module-validation-2

5c827e4

fix: add nil check

4151e01

fix: rename files

12205f5

Parkreiner changed the base branch from mes/module-validation to main May 2, 2025 15:32

Parkreiner added 3 commits May 2, 2025 15:34

Merge branch 'main' into mes/module-validation-2

091124d

fix: remove unnecessary type

dda33d7

fix: update type for validation phase

2180f4b

Parkreiner merged commit ba6fea8 into main May 2, 2025
3 checks passed

Parkreiner deleted the mes/module-validation-2 branch May 2, 2025 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add logic to validate all modules #11

chore: add logic to validate all modules #11

Parkreiner commented Apr 19, 2025 •

edited

Loading

Parkreiner Apr 21, 2025

f0ssel Apr 30, 2025

Parkreiner Apr 21, 2025

Parkreiner Apr 21, 2025

Parkreiner Apr 21, 2025

Parkreiner Apr 21, 2025

bcpeinhardt left a comment

f0ssel left a comment

f0ssel Apr 30, 2025

f0ssel Apr 30, 2025

Parkreiner May 2, 2025

f0ssel May 2, 2025

Parkreiner May 2, 2025 •

edited

Loading

f0ssel Apr 30, 2025

f0ssel Apr 30, 2025

f0ssel commented Apr 30, 2025

		var errs []error
		errs = append(errs, validateReadmeBody(trimmed)...)

chore: add logic to validate all modules #11

chore: add logic to validate all modules #11

Conversation

Parkreiner commented Apr 19, 2025 • edited Loading

Changes made

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcpeinhardt left a comment

Choose a reason for hiding this comment

f0ssel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f0ssel commented Apr 30, 2025

Parkreiner commented Apr 19, 2025 •

edited

Loading

Parkreiner May 2, 2025 •

edited

Loading