implement schema validation for vendoring and CLI schemas #1147
Conversation
…o-support-dynamic-schema
…ynamic-schema' of https://github.com/cloudposse/atmos into feature/dev-3082-update-atmosconfiguration-to-support-dynamic-schema
…o-support-dynamic-schema
…ynamic-schema' of https://github.com/cloudposse/atmos into feature/dev-3082-update-atmosconfiguration-to-support-dynamic-schema
…o-support-dynamic-schema
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
Co-authored-by: Erik Osterman (CEO @ Cloud Posse) <erik@cloudposse.com>
…o-support-dynamic-schema
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/markdown/atmos_validate_schema_usage.md (4)
1-2: Refine list formatting for clarity.Consider using a colon or em dash after the list items for a cleaner look:
-- Validate all the schemas +- Validate all the schemas:🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - Validate all the schemas ```bash $ at...(DASH_RULE)
3-5: Remove shell prompt for command-only examples.For documentation clarity and markdownlint compliance, omit the
$prompt unless you're showing command output:-```bash - $ atmos validate schema -``` +```bash +atmos validate schema +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
4-4: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
7-8: Refine list formatting for clarity.Same as above, use a colon or em dash for consistency:
-- Validate specific schema +- Validate specific schema:
9-11: Add language specifier and remove shell prompt.Specify the language for the code block and remove the
$for consistency:-``` - $ atmos validate schema [schemaToValidate] -``` +```bash +atmos validate schema [schemaToValidate] +```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
10-10: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
.golangci.yml(1 hunks)cmd/markdown/atmos_validate_schema_usage.md(1 hunks)go.mod(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- .golangci.yml
🧰 Additional context used
🪛 LanguageTool
cmd/markdown/atmos_validate_schema_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - Validate all the schemas ```bash $ at...
(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
cmd/markdown/atmos_validate_schema_usage.md
4-4: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
9-9: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
10-10: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/markdown/atmos_validate_schema_usage.md (1)
3-5: Polish command examples for consistency and copy-paste ease.Consider removing the
$prompt from the command examples so users can copy-paste directly. Also, addbashas the language specifier to the second code block for clarity.Here's a suggested tweak:
-```bash - $ atmos validate schema -``` +```bash +atmos validate schema +``` -``` - $ atmos validate schema [schemaToValidate] -``` +```bash +atmos validate schema [schemaToValidate] +```Also applies to: 9-11
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
4-4: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
.golangci.yml(1 hunks)cmd/markdown/atmos_validate_schema_usage.md(1 hunks)go.mod(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .golangci.yml
- go.mod
🧰 Additional context used
🪛 LanguageTool
cmd/markdown/atmos_validate_schema_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: - Validate all the schemas ```bash $ at...
(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
cmd/markdown/atmos_validate_schema_usage.md
4-4: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
9-9: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
10-10: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
…-for-vendoring-and-2
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/validate_schema.go (3)
3-11: Clean up redundant imports for utils package.You're importing the same utils package twice - once as the full name and once with an alias 'u'. This creates unnecessary confusion.
import ( "errors" log "github.com/charmbracelet/log" "github.com/cloudposse/atmos/internal/exec" - "github.com/cloudposse/atmos/pkg/utils" u "github.com/cloudposse/atmos/pkg/utils" "github.com/spf13/cobra" )
13-13: Complete the comment with a period.The static analysis tool correctly identified that this comment should end with a period to maintain consistency with Go documentation standards.
- // ValidateSchemaCmd represents the 'atmos validate schema' command. + // ValidateSchemaCmd represents the 'atmos validate schema' command.
55-57: Consider handling flag parsing errors consistently.While you've indicated the flag parsing error handling isn't required, it would be better for consistency and robustness to handle potential errors.
if cmd.Flags().Changed("schemas-atmos-manifest") { - schema, _ = cmd.Flags().GetString("schemas-atmos-manifest") + var err error + schema, err = cmd.Flags().GetString("schemas-atmos-manifest") + if err != nil { + log.Error("Failed to parse schema flag", "error", err) + u.OsExit(1) + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/validate_schema.go(1 hunks)website/docs/cli/commands/validate/validate-schema.mdx(1 hunks)website/src/components/Screengrabs/atmos-validate-schema--help.html(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- website/docs/cli/commands/validate/validate-schema.mdx
- website/src/components/Screengrabs/atmos-validate-schema--help.html
🧰 Additional context used
🧠 Learnings (1)
cmd/validate_schema.go (1)
Learnt from: samtholiya
PR: cloudposse/atmos#1147
File: internal/exec/validate_schema.go:42-57
Timestamp: 2025-04-11T22:06:46.999Z
Learning: The "ExecuteAtmosValidateSchemaCmd" function in internal/exec/validate_schema.go has been reviewed and confirmed to have acceptable cognitive complexity despite static analysis warnings. The function uses a clean structure with only three if statements for error handling and delegates complex operations to helper methods.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (2)
cmd/validate_schema.go (2)
25-71: The command implementation is well structured and robust.The command properly separates concerns and provides clear error handling paths. The retrieved learning confirms that the ExecuteAtmosValidateSchemaCmd function has acceptable complexity despite static analysis warnings, as it delegates complex operations to helper methods.
73-76: Flag definition and command registration looks good.The flag name matches what's being checked in the command execution and has a clear description. The command is properly registered with the parent command.
…-for-vendoring-and-2
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/datafetcher/schema/stacks/stack-config/1.0.json (1)
380-389:⚠️ Potential issueAdd
!includesupport to scalar definitions.Per the project conventions, any definition should allow an include string (
!include). The"command"and"component"definitions currently only allow plain strings. Update them to aoneOfthat supports both a!includereference and a normal string.Example diff:
"command": { - "title": "command", - "description": "Command to execute", - "type": "string" + "title": "command", + "description": "Command to execute", + "oneOf": [ + { "type": "string", "pattern": "^!include" }, + { "type": "string" } + ] }, "component": { - "title": "component", - "description": "Component section", - "type": "string" + "title": "component", + "description": "Component section", + "oneOf": [ + { "type": "string", "pattern": "^!include" }, + { "type": "string" } + ] },
🧹 Nitpick comments (2)
pkg/datafetcher/schema/stacks/stack-config/1.0.json (2)
2-4: Include version in$idfor clarity and conflict avoidance.Using a generic
$idmay lead to collisions if multiple schema versions are published. Consider appending the version to the$id, e.g.:-$id": "https://json.schemastore.org/atmos-manifest.json", +$id": "https://json.schemastore.org/atmos-manifest-1.0.json",
38-38: Consider restricting unknown top-level properties.Currently
additionalProperties: trueat the root allows any extra keys. To enforce strict schema validation, you may want to setadditionalProperties: falseso that only known sections are permitted. If you need to allow extensions, you could use a pattern or custom extension mechanism instead.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/datafetcher/schema/config/global/1.0.json(1 hunks)pkg/datafetcher/schema/stacks/stack-config/1.0.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/datafetcher/schema/config/global/1.0.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
|
These changes were released in v1.174.0. |
what
why
references
Summary by CodeRabbit