feat(cli): Add check-only option to render#170
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a --check-only mode to azldev component render so CI can detect whether the checked-in rendered-spec output would change, without mutating the existing output directory.
Changes:
- Added
--check-onlyflag andRenderResult.Changedreporting, with drift detection by diffing staging output against on-disk output. - Changed
--clean-stalebehavior to prune orphan rendered-spec component directories (instead of wiping the entire output dir contents), and added orphan detection for check-only runs. - Updated command wiring tests and regenerated the CLI reference doc to include the new flag.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/app/azldev/cmds/component/render.go | Implements --check-only, per-component drift detection, orphan pruning, and marker validation logic. |
| internal/app/azldev/cmds/component/render_test.go | Verifies the new --check-only flag is registered on the command. |
| internal/app/azldev/cmds/component/render_internal_test.go | Adds internal tests for orphan pruning, directory diff drift detection, and check-only marker validation behavior. |
| docs/user/reference/cli/azldev_component_render.md | Regenerated CLI docs to include --check-only. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
internal/app/azldev/cmds/component/render.go:1137
- For components that error before diffRenderedOutput runs (e.g. missing/failed mock results), finishComponentRender returns changed=false, but writeFailureMarkers may still delete existing output (when allowOverwrite) and/or write the RENDER_FAILED marker. In non-'--check-only' runs this can cause Result.Changed to incorrectly report no on-disk change for errored components. Consider setting result.Changed when writing/removing markers (or otherwise incorporating marker writes into the Changed calculation).
if allowOverwrite {
if removeErr := fileSystem.RemoveAll(result.OutputDir); removeErr != nil {
slog.Debug("Failed to clean output before writing error marker",
"path", result.OutputDir, "error", removeErr)
}
}
writeRenderErrorMarker(fileSystem, result.OutputDir)
}
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if allowOverwrite { | ||
| if removeErr := fileSystem.RemoveAll(result.OutputDir); removeErr != nil { | ||
| slog.Debug("Failed to clean output before writing error marker", |
| return errors.New("--clean-stale requires -a (render all components)") | ||
| } | ||
|
|
||
| if options.OutputDirExplicit && !options.Force { |
reubeno
left a comment
There was a problem hiding this comment.
I'm a bit worried about the length and complexity of render.go; not to block here, but are there some utility or abstractions that could be removed?
Could we simplify logic by batching up "transactions" of the changes we'd make, and then only apply them in non-check mode (to avoid parallel code paths)? Can we extract out utility helpers that raise the altitude above file/dir operations and more on a "rendered spec cache", which is effectively what base/specs does? Sort of like what you did with the lock store?
No description provided.