Skip to content

fix(cli): reconstruct service Details from CSV Engine/Deployment columns (closes #572) - #601

Merged
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/issue-572-csv-input-engine-deployment
May 20, 2026
Merged

fix(cli): reconstruct service Details from CSV Engine/Deployment columns (closes #572)#601
cristim merged 1 commit into
feat/multicloud-web-frontendfrom
fix/issue-572-csv-input-engine-deployment

Conversation

@cristim

@cristim cristim commented May 20, 2026

Copy link
Copy Markdown
Member

What

Make --input-csv purchasing work for RDS by reconstructing rec.Details from the CSV's Engine and Deployment columns.

Why

loadRecommendationsFromCSVparseCSVRecord parsed only Service, Region, ResourceType, Account, AccountName, Term, PaymentOption, Count, EstimatedSavings. It never read Engine/Deployment, so rec.Details stayed nil. The RDS purchase path can't proceed without it:

// providers/aws/services/rds/client.go  findOfferingID
details, ok := rec.Details.(*common.DatabaseDetails)
if !ok || details == nil {
    return "", fmt.Errorf("invalid service details for RDS")
}

So every RDS row failed with invalid service details for RDS under --input-csv … --purchase, before any AWS call. This blocks the edit-the-CSV-then-purchase workflow (the tool has no in-flow exclusion flag).

Change

  • parseCSVRecord reconstructs Details from the Engine/Deployment columns, mirroring the writer (extractEngine/extractDeployment emit DatabaseDetails / CacheDetails / ComputeDetails), so a CSV the tool wrote round-trips losslessly. Guarded on a non-empty Engine, so minimal CSVs and Savings Plans rows keep their previous nil-Details behavior. Engine is stored in Cost Explorer format ("Aurora MySQL"); findOfferingID already normalizes it.
  • parseCSVRecords skips the trailing TOTAL summary row, so feeding the tool its own output no longer parses TOTAL as a bogus recommendation ("Service client not yet implemented for TOTAL").

Out of scope (by design)

Multi-account purchasing from a single CSV. Purchases run centrally in the --profile account; the Account column stays informational. This is the intended behavior, not a gap.

Testing

  • go build ./cmd/..., go vet ./cmd/ clean.
  • go test ./cmd/ → 720 pass.
  • New table cases in TestLoadRecommendationsFromCSV:
    • Engine + Deployment reconstruct *DatabaseDetails (Aurora MySQL/single-az, MySQL/multi-az), *CacheDetails (ElastiCache), *ComputeDetails (EC2 platform).
    • No Engine column leaves Details nil (Savings Plans / minimal CSV).
    • TOTAL summary row is skipped.
  • Manual dry-run (--input-csv … --profile shift-prd, no --purchase): loads 22 recs / 85 instances cleanly and no longer emits the spurious "Processing TOTAL" block.

Closes #572

Summary by CodeRabbit

  • Bug Fixes
    • CSV import now correctly ignores summary rows, preventing footer data from being interpreted as valid recommendations
    • Improved recommendation data reconstruction from CSV imports, properly mapping Engine and Deployment columns to the appropriate service-specific details for database, cache, and compute recommendations

Review Change Stack

The --input-csv loader parsed only Service/Region/ResourceType/Count/etc.
and never read the Engine or Deployment columns, leaving rec.Details nil.
RDS findOfferingID requires a *DatabaseDetails (engine + Multi-AZ) to look
up an RI offering, so every RDS row failed with "invalid service details
for RDS" when purchasing from a CSV.

parseCSVRecord now reconstructs Details from the Engine/Deployment columns,
mirroring the writer (DatabaseDetails/CacheDetails/ComputeDetails) so a CSV
the tool wrote round-trips losslessly. Guarded on a non-empty Engine so
minimal CSVs and Savings Plans rows keep their previous nil-Details behavior.

Also skip the trailing TOTAL summary row so feeding the tool its own output
no longer parses TOTAL as a bogus recommendation with an unknown service.

Multi-account purchasing stays intentionally out of scope: the Account
column remains informational and purchases run centrally in the --profile
account.

Closes #572
@cristim cristim added triaged Item has been triaged priority/p1 Next up; this sprint severity/medium Moderate harm urgency/this-sprint Within the current sprint impact/internal Team-internal only effort/s Hours type/bug Defect labels May 20, 2026
@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5ff70330-ac16-4ff6-8570-97a8cfff4bd9

📥 Commits

Reviewing files that changed from the base of the PR and between bc8831b and a23eaf3.

📒 Files selected for processing (2)
  • cmd/multi_service_csv.go
  • cmd/multi_service_csv_test.go

📝 Walkthrough

Walkthrough

CSV loader now reconstructs typed Details payloads (DatabaseDetails, CacheDetails, ComputeDetails) from Engine and Deployment columns when present, enabling service-specific purchase flows. Parser also filters out TOTAL summary rows to avoid parsing tool output as recommendations.

Changes

CSV Details Reconstruction

Layer / File(s) Summary
Service Details reconstruction from Engine/Deployment
cmd/multi_service_csv.go
parseCSVRecord now builds typed Details from Engine and Deployment columns: RDS→DatabaseDetails (Engine, AZConfig), ElastiCache→CacheDetails (Engine, NodeType from ResourceType), EC2→ComputeDetails (Platform from Engine). Nil when Engine is absent to preserve behavior for minimal CSVs.
TOTAL row filtering
cmd/multi_service_csv.go
loadRecommendationsFromCSV skips records where Service equals TOTAL, preventing CSV export footer from being parsed as a recommendation.
Test coverage
cmd/multi_service_csv_test.go
Validates Engine/Deployment columns trigger correct Details reconstruction for RDS, ElastiCache, and EC2; verifies Details stays nil without Engine; confirms TOTAL rows are ignored.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

Suggested labels

impact/all-users, effort/m

Poem

A CSV walks in with tales to tell,
Engine and Deployment serve them well,
Details bloom where TOTAL would fall—
Now purchases work! No more wall. 🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: reconstructing service Details from CSV Engine/Deployment columns, which is the primary objective of the changeset.
Linked Issues check ✅ Passed The PR fully addresses all coding requirements from issue #572: Engine/Deployment parsing is restored, appropriate Details types are reconstructed for RDS/ElastiCache/EC2, non-empty Engine guards minimal CSVs, trailing TOTAL rows are skipped, and tests validate all scenarios.
Out of Scope Changes check ✅ Passed All changes are directly related to the linked issue objectives: CSV parsing logic for Engine/Deployment, Details reconstruction, TOTAL row filtering, and corresponding test coverage. No unrelated modifications are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-572-csv-input-engine-deployment

Comment @coderabbitai help to get the list of available commands and usage tips.

@cristim

cristim commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cristim
cristim merged commit cf78ff1 into feat/multicloud-web-frontend May 20, 2026
4 checks passed
@cristim
cristim deleted the fix/issue-572-csv-input-engine-deployment branch May 20, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/s Hours impact/internal Team-internal only priority/p1 Next up; this sprint severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-sprint Within the current sprint

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant