feat(aws/recs): populate ComputeDetails.VCPU/MemoryGB via DescribeInstanceTypes (closes #218) by cristim · Pull Request #816 · LeanerCloud/cloud-commitments-cli · GitHub
Skip to content

feat(aws/recs): populate ComputeDetails.VCPU/MemoryGB via DescribeInstanceTypes (closes #218) - #816

Merged
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/218-wave6
Jun 6, 2026
Merged

feat(aws/recs): populate ComputeDetails.VCPU/MemoryGB via DescribeInstanceTypes (closes #218)#816
cristim merged 2 commits into
feat/multicloud-web-frontendfrom
fix/218-wave6

Conversation

@cristim

@cristim cristim commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds a per-Client instance-type SKU catalogue fetched once per lifetime via ec2:DescribeInstanceTypes (paginated), cached with sync.Once
  • Populates ComputeDetails.VCPU and MemoryGB in parseEC2Details from the catalogue; falls back to 0 on cache miss or fetch failure (graceful degradation, mirrors Azure PR feat(azure/recs): populate ComputeDetails.VCPU/MemoryGB via cached SKU lookup (closes #217) #810)
  • Threads context.Context through parseRecommendations -> parseRecommendationDetail -> parseServiceSpecificDetails -> service parsers to carry cancellation to the catalogue walk

IAM

ec2:DescribeInstanceTypes is covered by the existing ec2:Describe* wildcard in the deploy SAs; no new policy statement required.

Test plan

  • go build ./... clean
  • go test github.com/LeanerCloud/CUDly/providers/aws/recommendations/... -- 293 tests pass (280 before + 13 new)
  • TestExtractInstanceTypeSKUEntry -- field extraction from InstanceTypeInfo
  • TestFetchInstanceTypeCatalogue_PopulatesMap -- paginator walk produces correct map
  • TestFetchInstanceTypeCatalogue_PageError -- nil on page error (graceful degradation)
  • TestFetchInstanceTypeCatalogue_ContextCanceled -- nil when ctx already canceled (ctx_cancel_terminal)
  • TestInstanceTypeLookup_CachedOnce -- pager factory called exactly once per client lifetime (N+1 invariant)
  • TestParseEC2Details_VCPUAndMemoryPopulated -- VCPU/MemoryGB populated from catalogue
  • TestParseEC2Details_CatalogueMiss -- 0/0 on miss, conversion succeeds
  • TestParseEC2Details_NoCatalogueConfigured -- 0/0 when no factory set, conversion succeeds

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/this-quarter Within the quarter impact/few Limited audience effort/m Days type/feat New capability labels May 28, 2026
@coderabbitai

coderabbitai Bot commented May 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 28, 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.

This was referenced May 28, 2026
Merged
Merged
@cristim

cristim commented May 28, 2026

Copy link
Copy Markdown
Member Author

Coordination note: PR #842 (closes #99) implements the same DescribeInstanceTypes VCPU/MemoryGB population path independently. The two PRs are effectively duplicates. Whichever lands first should be merged; the other should be closed (or rebased to a tightening-only delta). Both currently open; flagging so the merger doesn't accidentally land both.

@cristim

cristim commented May 30, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented May 30, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Full review triggered.

@cristim

cristim commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 1, 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 added a commit that referenced this pull request Jun 3, 2026
@cristim
feat(frontend/recs): surface VCPU + MemoryGB in Capacity column (closes
123fe91 
#219)

Add a new "Capacity" column to the recommendations table immediately
after "Resource Type". The column renders "N vCPU / M GB" when the
backend populates ComputeDetails.VCPU and ComputeDetails.MemoryGB
(populated by PRs #810/#816/#833 for Azure/AWS/GCP). Recs without those
fields show a dash, matching the feedback_nullable_not_zero rule.

Changes:
- types.ts: add vcpu/memory_gb optional fields to LocalRecommendation
- state.ts: add 'capacity' to RecommendationsColumnId
- recommendations.ts: add COLUMN_DEFS entry, formatCapacity() helper,
  and cases in categoricalCellValue/numericCellValue/displayPrecision/
  renderColumnCell; all innerHTML values go through escapeHtml()
- tests: +11 tests (formatCapacity unit, capacity filter, column count
  and position assertions); update 5 hardcoded td-index/column-list
  assertions shifted by the new column
cristim added a commit that referenced this pull request Jun 3, 2026
@cristim
feat(frontend/recs): surface VCPU + MemoryGB in Capacity column (closes
178aea5 
#219)

Add a new "Capacity" column to the recommendations table immediately
after "Resource Type". The column renders "N vCPU / M GB" when the
backend populates ComputeDetails.VCPU and ComputeDetails.MemoryGB
(populated by PRs #810/#816/#833 for Azure/AWS/GCP). Recs without those
fields show a dash, matching the feedback_nullable_not_zero rule.

Changes:
- types.ts: add vcpu/memory_gb optional fields to LocalRecommendation
- state.ts: add 'capacity' to RecommendationsColumnId
- recommendations.ts: add COLUMN_DEFS entry, formatCapacity() helper,
  and cases in categoricalCellValue/numericCellValue/displayPrecision/
  renderColumnCell; all innerHTML values go through escapeHtml()
- tests: +11 tests (formatCapacity unit, capacity filter, column count
  and position assertions); update 5 hardcoded td-index/column-list
  assertions shifted by the new column
cristim added a commit that referenced this pull request Jun 3, 2026
@cristim
feat(frontend/recs): surface VCPU + MemoryGB in Capacity column (closes
462f9ab 
#219)

Add a new "Capacity" column to the recommendations table immediately
after "Resource Type". The column renders "N vCPU / M GB" when the
backend populates ComputeDetails.VCPU and ComputeDetails.MemoryGB
(populated by PRs #810/#816/#833 for Azure/AWS/GCP). Recs without those
fields show a dash, matching the feedback_nullable_not_zero rule.

Changes:
- types.ts: add vcpu/memory_gb optional fields to LocalRecommendation
- state.ts: add 'capacity' to RecommendationsColumnId
- recommendations.ts: add COLUMN_DEFS entry, formatCapacity() helper,
  and cases in categoricalCellValue/numericCellValue/displayPrecision/
  renderColumnCell; all innerHTML values go through escapeHtml()
- tests: +11 tests (formatCapacity unit, capacity filter, column count
  and position assertions); update 5 hardcoded td-index/column-list
  assertions shifted by the new column
@cristim

cristim commented Jun 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 1 seconds before sending another message.

cristim added 2 commits June 5, 2026 13:17
@cristim
feat(aws/recs): populate ComputeDetails.VCPU/MemoryGB via DescribeIns…
221aca8 
…tanceTypes (closes #218)

Add a per-Client instance-type SKU catalogue fetched once per scheduler
tick via ec2:DescribeInstanceTypes (paginated, sync.Once cached).
parseEC2Details now enriches ComputeDetails.VCPU and MemoryGB from the
catalogue; catalogue miss or fetch failure leaves both fields at 0
(graceful degradation, mirrors Azure PR #810).

Context is threaded through parseRecommendations/parseRecommendationDetail/
parseServiceSpecificDetails so the paginator walk honours cancellation
(ctx_cancel_terminal pattern).

IAM: ec2:DescribeInstanceTypes is covered by the existing ec2:Describe*
wildcard; no new policy statement required.

Tests: 293 pass (+13 new) including one-call-per-lifetime invariant,
page-error graceful degradation, and ctx-cancel short-circuit.
@cristim
fix(ci): resolve gofmt and gocyclo failures in AWS recommendations
d756be7 
Fix two pre-commit failures:
- sku_test.go: gofmt reorders imports into canonical alphabetical order
- parser_services.go: extract resolveEC2Tenancy, resolveEC2Scope, and
  enrichFromCatalogue helpers from parseEC2Details; complexity drops from
  14 to 7, clearing the gocyclo hook
@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

Rebased on feat/multicloud-web-frontend. Resolved conflict in providers/aws/recommendations/parser_services.go and parser_services_test.go (parseMemoryDBDetails signature: kept fail-loudly semantics from base, restored context.Context param for serviceParserFunc type consistency).

@cristim

cristim commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

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 commented Jun 5, 2026

Copy link
Copy Markdown
Member Author

@cristim
cristim merged commit 209db78 into feat/multicloud-web-frontend Jun 6, 2026
4 checks passed
cristim added a commit that referenced this pull request Jun 8, 2026
@cristim
test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled …
ac9fad7 
…ctx; drop em-dashes

The ComputeDetails VCPU/MemoryGB implementation landed via the merged
duplicate PR #816 (closes #218). This commit grafts the two test-coverage
gaps that #816 missed, plus a comment cleanup:

- TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but
  SizeInMiB nil" -- expects VCPU populated, MemGB 0.0.
- TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount
  remains 0 after a pre-canceled ctx returns nil (the stub already tracks the
  count via atomic; this makes the no-NextPage contract explicit).
- Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the
  client.go instanceTypeLookup comment.

parser_services.go em-dashes left for a follow-up (out of scope here).

Closes #99
cristim added a commit that referenced this pull request Jun 19, 2026
@cristim
test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled …
a751026 
…ctx; drop em-dashes

The ComputeDetails VCPU/MemoryGB implementation landed via the merged
duplicate PR #816 (closes #218). This commit grafts the two test-coverage
gaps that #816 missed, plus a comment cleanup:

- TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but
  SizeInMiB nil" -- expects VCPU populated, MemGB 0.0.
- TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount
  remains 0 after a pre-canceled ctx returns nil (the stub already tracks the
  count via atomic; this makes the no-NextPage contract explicit).
- Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the
  client.go instanceTypeLookup comment.

parser_services.go em-dashes left for a follow-up (out of scope here).

Closes #99
cristim added a commit that referenced this pull request Jul 5, 2026
@cristim
test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled …
f7cb446 
…ctx; drop em-dashes

The ComputeDetails VCPU/MemoryGB implementation landed via the merged
duplicate PR #816 (closes #218). This commit grafts the two test-coverage
gaps that #816 missed, plus a comment cleanup:

- TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but
  SizeInMiB nil" -- expects VCPU populated, MemGB 0.0.
- TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount
  remains 0 after a pre-canceled ctx returns nil (the stub already tracks the
  count via atomic; this makes the no-NextPage contract explicit).
- Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the
  client.go instanceTypeLookup comment.

parser_services.go em-dashes left for a follow-up (out of scope here).

Closes #99
cristim added a commit that referenced this pull request Jul 10, 2026
@cristim
test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled …
fe2c696 
…ctx; drop em-dashes

The ComputeDetails VCPU/MemoryGB implementation landed via the merged
duplicate PR #816 (closes #218). This commit grafts the two test-coverage
gaps that #816 missed, plus a comment cleanup:

- TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but
  SizeInMiB nil" -- expects VCPU populated, MemGB 0.0.
- TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount
  remains 0 after a pre-canceled ctx returns nil (the stub already tracks the
  count via atomic; this makes the no-NextPage contract explicit).
- Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the
  client.go instanceTypeLookup comment.

parser_services.go em-dashes left for a follow-up (out of scope here).

Closes #99
cristim added a commit that referenced this pull request Jul 17, 2026
@cristim
test(aws/recs): cover SizeInMiB-nil + assert no NextPage on canceled …
63d1805 
…ctx; drop em-dashes

The ComputeDetails VCPU/MemoryGB implementation landed via the merged
duplicate PR #816 (closes #218). This commit grafts the two test-coverage
gaps that #816 missed, plus a comment cleanup:

- TestExtractInstanceTypeSKUEntry: add sub-case "MemoryInfo present but
  SizeInMiB nil" -- expects VCPU populated, MemGB 0.0.
- TestFetchInstanceTypeCatalogue_ContextCanceled: assert that pager.callCount
  remains 0 after a pre-canceled ctx returns nil (the stub already tracks the
  count via atomic; this makes the no-NextPage contract explicit).
- Replace em-dashes (U+2014) with "--" in sku.go warn messages and in the
  client.go instanceTypeLookup comment.

parser_services.go em-dashes left for a follow-up (out of scope here).

Closes #99
@cristim
cristim deleted the fix/218-wave6 branch July 27, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

effort/m Days impact/few Limited audience priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/this-quarter Within the quarter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cristim