atmos list vendor #1192
atmos list vendor #1192
Conversation
This commit introduces the `list vendor` command to list vendor configurations, including component and vendor manifests. It also adds: - Filtering by stack - Support for JSON, YAML, CSV, TSV, and table output formats - Custom column configuration
|
Warning This PR exceeds the recommended limit of 1,000 lines.Large PRs are difficult to review and may be rejected due to their size. Please verify that this PR does not address multiple issues. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1192 +/- ##
==========================================
+ Coverage 32.89% 33.26% +0.36%
==========================================
Files 223 226 +3
Lines 23791 24373 +582
==========================================
+ Hits 7827 8107 +280
- Misses 14798 15085 +287
- Partials 1166 1181 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThis change introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Config
participant ListVendor
participant Formatter
User->>CLI: Run 'atmos list vendor' [with flags]
CLI->>Config: Initialize and validate configuration
CLI->>ListVendor: FilterAndListVendor(config, options)
ListVendor->>ListVendor: Discover component/vendor manifests
ListVendor->>ListVendor: Apply filters (e.g., stack pattern)
ListVendor->>Formatter: Format output (table/JSON/CSV/TSV)
Formatter-->>ListVendor: Formatted output string
ListVendor-->>CLI: Output string
CLI-->>User: Prints formatted vendor configuration list
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
pkg/list/list_vendor.go (1)
71-111: 🛠️ Refactor suggestionHigh cyclomatic complexity – pull common branches into helpers
formatVendorOutputnow weighs in at 12 decision points (same number flagged by Advanced‑Security). Most of that stems from three separate switches that partially duplicate each other. You can shave the complexity and make the intent clearer by:
- Creating a small helper that returns the
datamap for a given format.- Moving the CSV/TSV special‑casing into its own formatter.
- Leaning on
format.NewFormatterfor everything else.-func formatVendorOutput(rows []map[string]interface{}, customHeaders []string, options *FilterOptions) (string, error) { - ... - switch options.FormatStr { - case "table": - if formatOpts.TTY { - return renderVendorTableOutput(customHeaders, rows), nil - } - case "csv": - return buildVendorCSVTSV(customHeaders, rows, ","), nil - case "tsv": - return buildVendorCSVTSV(customHeaders, rows, "\t"), nil - } - - output, err := formatter.Format(data, formatOpts) - ... -} +func formatVendorOutput(rows []map[string]interface{}, headers []string, opt *FilterOptions) (string, error) { + getData := func() map[string]interface{} { + withHeader := opt.FormatStr == "table" || opt.FormatStr == "json" + return buildVendorDataMap(rows, withHeader) + } + + switch opt.FormatStr { + case "csv": + return buildVendorCSVTSV(headers, rows, ","), nil + case "tsv": + return buildVendorCSVTSV(headers, rows, "\t"), nil + case "table": + if term.IsTerminal(int(os.Stdout.Fd())) { + return renderVendorTableOutput(headers, rows), nil + } + } + + formatter, err := format.NewFormatter(format.Format(opt.FormatStr)) + if err != nil { + return "", err + } + return formatter.Format(getData(), format.FormatOptions{ + Format: format.Format(opt.FormatStr), + Delimiter: opt.Delimiter, + TTY: term.IsTerminal(int(os.Stdout.Fd())), + CustomHeaders: headers, + }) +}This drops cyclomatic complexity to ~6, satisfying the linter and making later maintenance painless.
🧹 Nitpick comments (8)
cmd/markdown/atmos_list_vendor_usage.md (1)
1-30: Documentation needs formatting improvements.The documentation provides good examples for the new command but has formatting issues:
- Use proper markdown list syntax (e.g.,
-or*) instead of en-dashes- Add language specifiers to code blocks (e.g., ```bash)
- Remove dollar signs from command examples or show output according to markdown best practices
-– List all vendor configurations -``` - $ atmos list vendor -``` +- List all vendor configurations +```bash +atmos list vendor +``` -– List vendor configurations with a specific output format -``` - $ atmos list vendor --format json -``` +- List vendor configurations with a specific output format +```bash +atmos list vendor --format json +```🧰 Tools
🪛 LanguageTool
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all vendor configurations ``` $ a...(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...gurations$ atmos list vendor– List vendor configurations with a speci...(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format json ``` – List vendor configurations filtered by ...(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list vendor --stack "vpc*" ``` – List vendor configurations with comma-s...(DASH_RULE)
[typographical] ~20-~20: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format csv ``` – List vendor configurations with tab-sep...(DASH_RULE)
[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format tsv ``` – List vendor configurations with a custo...(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
2-2: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
3-3: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
7-7: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
8-8: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
12-12: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
13-13: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
17-17: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
18-18: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
22-22: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
23-23: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
27-27: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
28-28: Dollar signs used before commands without showing output
null(MD014, commands-show-output)
pkg/list/format/table.go (2)
172-174: Exported function comment should match the function nameThe doc comment still says
createStyledTablebut the exported symbol is nowCreateStyledTable.
Please update the comment to start with the exact function name to satisfygolintand improve godoc.-// createStyledTable creates a styled table with headers and rows. +// CreateStyledTable creates a styled table with headers and rows.
173-173: Public helper could live in a dedicatedformatpackageExporting
CreateStyledTablefrom this file is handy, but it slightly blurs the boundary between “table helpers” and the higher‑level formatter types.
If you foresee re‑use across packages, consider moving it (and maybe the constants) into a tinypkg/ui/tableorpkg/list/format/tableutilpackage to keep the API surface tight.Purely optional, leaving here for future consideration.
cmd/list_vendor.go (1)
43-50: Delimiter flag is accepted even for non‑delimited formatsRight now a user can run
atmos list vendor --format json --delimiter "|", and the flag is silently ignored.
You might want to validate that--delimiteris only provided when--formatiscsvortsv, otherwise return a helpful error.if delimiterFlag != "" && formatFlag != "csv" && formatFlag != "tsv" { cmd.PrintErrln("The --delimiter flag is only valid with --format csv|tsv") return }pkg/list/list_vendor_format.go (1)
12-19: Duplicate sizing constants—consider centralising
DefaultTerminalWidthandMaxColumnWidthalready exist (or very similar ones do) inpkg/list/format/table.go.
Keeping two separate definitions risks them diverging. Importing the original constants (or exposing them in a shared package) will avoid magic numbers and keep behaviour consistent.pkg/list/list_vendor_test.go (1)
125-141: Minor duplication in assertionsThe table‑format test checks for
"Component"twice (lines 132 and 137).
Harmless, but you can drop one of them to keep the test lean.pkg/list/list_vendor.go (2)
113-132: Early‑return opportunity & duplicated validation
FilterAndListVendorvalidatesoptions.FormatStrtwice (once implicitly throughValidateFormat, and again when constructing the formatter insideformatVendorOutput). You can:
- Return early on invalid format right here.
- Skip the second validation (or make
formatVendorOutputassume it got a valid enum).Additionally, the linter report mentions an early‑return suggestion – replacing
if options.FormatStr == "" { options.FormatStr = string(format.FormatTable) } if err := format.ValidateFormat(options.FormatStr); err != nil { return "", err }with:
if options.FormatStr == "" { options.FormatStr = string(format.FormatTable) } else if err := format.ValidateFormat(options.FormatStr); err != nil { return "", err }eliminates the else branch entirely and silences that finding.
460-501: Avoid round‑tripping through YAML – unmarshal directly
readComponentManifestconverts amap[string]interface{}back to YAML only to unmarshal it again into a struct. That incurs extra allocations and loses the benefit ofyaml.UnmarshalStrict.Since
filetype.DetectFormatAndParseFilealready gives you a typedmap, decode straight intoComponentManifest:-var manifest schema.ComponentManifest -… -yamlData, err := yaml.Marshal(mapData) -… -if err := yaml.Unmarshal(yamlData, &manifest); err != nil { - … -} +var manifest schema.ComponentManifest +if err := mapstructure.Decode(mapData, &manifest); err != nil { + return nil, fmt.Errorf("error decoding component manifest: %w", err) +}(The
mapstructurepackage is already a transitive dependency in Atmos; if not, it’s lightweight.)
This halves the function’s CPU time and removes unnecessary imports.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/list_vendor.go(1 hunks)cmd/markdown/atmos_list_vendor_usage.md(1 hunks)pkg/list/format/formatter.go(1 hunks)pkg/list/format/table.go(3 hunks)pkg/list/list_vendor.go(1 hunks)pkg/list/list_vendor_format.go(1 hunks)pkg/list/list_vendor_test.go(1 hunks)pkg/schema/schema.go(1 hunks)tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/list_vendor.go (5)
pkg/schema/schema.go (2)
Command(527-537)ConfigAndStacksInfo(372-442)pkg/config/config.go (1)
InitCliConfig(25-62)pkg/list/list_values.go (1)
FilterOptions(55-64)pkg/list/list_vendor.go (1)
FilterAndListVendor(114-132)cmd/cmd_utils.go (1)
AddStackCompletion(715-720)
🪛 LanguageTool
cmd/markdown/atmos_list_vendor_usage.md
[typographical] ~1-~1: Consider using an em dash in dialogues and enumerations.
Context: – List all vendor configurations ``` $ a...
(DASH_RULE)
[typographical] ~5-~5: Consider using an em dash in dialogues and enumerations.
Context: ...gurations $ atmos list vendor – List vendor configurations with a speci...
(DASH_RULE)
[typographical] ~10-~10: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format json ``` – List vendor configurations filtered by ...
(DASH_RULE)
[typographical] ~15-~15: Consider using an em dash in dialogues and enumerations.
Context: ...$ atmos list vendor --stack "vpc*" ``` – List vendor configurations with comma-s...
(DASH_RULE)
[typographical] ~20-~20: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format csv ``` – List vendor configurations with tab-sep...
(DASH_RULE)
[typographical] ~25-~25: Consider using an em dash in dialogues and enumerations.
Context: ... $ atmos list vendor --format tsv ``` – List vendor configurations with a custo...
(DASH_RULE)
🪛 markdownlint-cli2 (0.17.2)
cmd/markdown/atmos_list_vendor_usage.md
2-2: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
3-3: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
7-7: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
8-8: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
12-12: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
13-13: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
17-17: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
18-18: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
22-22: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
23-23: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
27-27: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
28-28: Dollar signs used before commands without showing output
null
(MD014, commands-show-output)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build (windows-latest, windows)
- GitHub Check: Summary
🔇 Additional comments (7)
pkg/schema/schema.go (2)
770-777: Well-designed component manifest struct.The new
ComponentManifeststruct has a clean Kubernetes-like schema with appropriate YAML/JSON tags. The use of generic maps for metadata, spec, and vars fields provides flexibility for different component configurations.
783-783: Good extension of Vendor struct.The addition of the
Listfield to theVendorstruct properly supports the new vendor listing configuration capabilities. Theomitemptytag is appropriate for this optional field.tests/snapshots/TestCLICommands_atmos_describe_config.stdout.golden (1)
129-133: Snapshot correctly updated with new vendor list configuration.The test snapshot has been properly updated to reflect the schema changes, showing the new
liststructure with default empty values.pkg/list/format/formatter.go (1)
25-25: Good addition of CustomHeaders support.The new
CustomHeadersfield inFormatOptionsallows for flexible table header customization, which enhances the vendor listing capabilities.pkg/list/format/table.go (1)
72-82: Make sure customHeaders keep the first “Key” column in sync with the row builder
createRowsalways places the key (orKeyValue) as the first element of every row, butcreateHeadernow forwardscustomHeadersverbatim.
If a caller supplies a header slice that does not start with the “Key” column or whose length doesn’t matchlen(stackKeys)+1, the printed table will be mis‑aligned (data shifted under the wrong headings).Consider adding a sanity check such as:
+if len(customHeaders) != len(stackKeys)+1 || customHeaders[0] != "Key" { + return nil, errors.Errorf("customHeaders must start with \"Key\" and contain exactly %d elements", len(stackKeys)+1) +}so that the function fails fast instead of silently producing a garbled table.
pkg/list/list_vendor_format.go (1)
68-85:buildVendorDataMaplower‑cases all keys, not just column namesWhen
capitalizeKeysis false, you applystrings.ToLowerto every key in the row map—including custom column names that might already be lowercase, creating duplicates such astype→type(unchanged) butfolder→folder(unchanged) while losing original case information.If the intent is only to lowercase the column names (
ColumnNameComponent, etc.), that’s fine, but please document it.
Otherwise, consider a more selective rule or leave the keys untouched and let downstream formatters handle casing.pkg/list/list_vendor.go (1)
549-558:relativeManifestPathmay mislead whensource.Fileis relativeWhen
source.Fileis set, you keep it as‑is:relativeManifestPath := source.FileIf the vendor manifest lives in a nested directory, the resulting path will be relative to that directory, not to
atmosConfig.BasePath, causing confusing output.Consider normalising:
if !filepath.IsAbs(relativeManifestPath) { relativeManifestPath = filepath.Join(filepath.Dir(path), relativeManifestPath) } relativeManifestPath, _ = filepath.Rel(atmosConfig.BasePath, relativeManifestPath)This aligns the column with component manifests and prevents duplicate entries.
what
why
Evidence:
references
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
Refactor