Skip to content

feat(mdatagen): add gofmt validation to prevent template formatting regressions#14721

Open
atilsensalduz wants to merge 1 commit intoopen-telemetry:mainfrom
atilsensalduz:feature/mdatagen-gofmt-validation
Open

feat(mdatagen): add gofmt validation to prevent template formatting regressions#14721
atilsensalduz wants to merge 1 commit intoopen-telemetry:mainfrom
atilsensalduz:feature/mdatagen-gofmt-validation

Conversation

@atilsensalduz
Copy link

@atilsensalduz atilsensalduz commented Mar 6, 2026

###Summary

Adds a structural gofmt validation step to the mdatagen code generator that fails at generation time when a template produces output that format.Source() would reformat, rather than silently accepting the fix from a downstream goimports run. This catches real issues (wrong tab depth, missing spaces, incorrect indentation) while ignoring cosmetic differences such as blank lines, import grouping separators, and struct-field alignment. As a prerequisite, 13 template files that had pre-existing formatting issues are fixed so the new check passes.

###Why the existing CI did not catch template formatting issues

The CI go:generate step runs make gogenerate, which expands to:

go install ./cmd/mdatagen
go generate ./...
make fmt          # runs goimports across the whole module

Because goimports runs after go generate, any formatting mistake in a template (wrong tab depth, missing blank line between import groups, extra space, etc.) is silently corrected before the subsequent git diff --exit-code check. The diff that CI sees is always empty, so the template issue is never surfaced.
The fix makes the generator self-policing: generateFile() now compares the raw template output against what format.Source() (gofmt) would produce. If they differ in any meaningful way (tab depth, missing spaces, wrong indentation), the generation step fails immediately with an error pointing to the offending file, before goimports has a chance to mask the problem.

Fixes: #14698

@atilsensalduz atilsensalduz requested review from a team and dmitryax as code owners March 6, 2026 13:28
@dmathieu
Copy link
Member

dmathieu commented Mar 6, 2026

wrong tab depth, missing spaces, incorrect indentation

Your LLM is wrong. These are not "bugs".

I don't think this is the right approach.
In CI, we should execute the templates, ensure the git repository is still clean. And then run gofmt and ensure it's still clean.

Doing this kind of check in pure-go overly complicates things.

@atilsensalduz
Copy link
Author

Hi @dmathieu, thank you for the review.
As you know generateFile applies format.Source(), this means generated files are always gofmt-correct regardless of template quality. So we can't catch template formatting issues by inspecting generated files, and gofmt doesn't run directly on template files. To work around this, I attempted to catch formatting issues at runtime by comparing generated files against their gofmt-formatted counterparts before writing them to disk. That said, some helper logic is needed to ensure the comparison captures meaningful differences.

If the only viable path is detecting template formatting issues in CI, we need to remove format.Source() from the write path in generateFile (keeping it only for syntax validation), then add a CI step that runs gofmt -l on generated files and fails if any need formatting. This would require reordering CI so the gofmt check runs before make fmt/goimports, since goimports would otherwise mask the issues. The tradeoff is that this approach requires more extensive template fixes to ensure raw template output is already gofmt-correct.

If this direction sounds good, I'll proceed with it.

@dmathieu
Copy link
Member

dmathieu commented Mar 9, 2026

How about adding a -noformat option to mdatagen that would skip the formatting section? Then, we can generate code without formatting and check the repository is clean.

@atilsensalduz atilsensalduz force-pushed the feature/mdatagen-gofmt-validation branch 3 times, most recently from 07ab274 to d2a79df Compare March 10, 2026 17:26
@github-actions github-actions bot added the release:risky-change This change may affect the release label Mar 10, 2026
@atilsensalduz atilsensalduz force-pushed the feature/mdatagen-gofmt-validation branch 2 times, most recently from ec14c39 to d0861fe Compare March 10, 2026 17:28
Signed-off-by: atilsensalduz <atil.sensalduz@gmail.com>
@atilsensalduz atilsensalduz force-pushed the feature/mdatagen-gofmt-validation branch from d0861fe to f4dfb33 Compare March 10, 2026 17:30
@atilsensalduz
Copy link
Author

Hey @dmathieu , thanks for the recommendation. That’s definitely a better approach, simple and clean. I’ve updated the PR. Right now make check-generate-fmt produces changes in about 130 generated files, so I’m not sure how we should proceed. 😅 I’ll try fixing the templating issues, but before going further I wanted to get your opinion.

Copy link
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

This needs tests.

if err != nil {
return nil, err
}
noformat := os.Getenv("MDATAGEN_NOFORMAT") == "1"
Copy link
Member

Choose a reason for hiding this comment

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

We don't use env vars to set flags anywhere within mdatagen. I don't think starting this pattern is a good fit for this PR.

Suggested change
noformat := os.Getenv("MDATAGEN_NOFORMAT") == "1"
var noFormat bool

@dmathieu
Copy link
Member

I would open a PR fixing all the issues independently from this one (link to here in that new PR), so both can be reviewed independently.

$(MAKE) fmt

.PHONY: check-generate-fmt
check-generate-fmt:
Copy link
Member

Choose a reason for hiding this comment

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

We will need to run this on CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd/mdatagen release:risky-change This change may affect the release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure templates are properly tab-formatted

2 participants