update-provider-table

[yaacov]

Summary by CodeRabbit

• Bug Fixes

  • Resource counts (VMs and networks) are now consistently displayed in provider inventory listings.
  • Improved reliability of resource counting across different provider configurations.

• Refactor

  • Standardized provider listing display logic to ensure uniform columns across all provider types.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Refactors provider inventory listing from provider-specific column construction to a unified, resource-type-agnostic approach. Introduces field normalization that maps inventory fields to resource types and dynamically counts missing resources. The core logic now consistently populates provider item fields through integrated normalization throughout the listing flow.

Changes

Cohort / File(s)

Summary

Provider Inventory Listing Refactor pkg/cmd/get/provider/list.go

Replaces ad-hoc per-provider column logic with unified approach. Introduces normalizeProviderInventory, countProviderResources, getProviders, and getSpecificProvider functions. Maps fields to resource types via getProviderRelevantFields. Simplifies getDynamicInventoryColumns to return fixed, provider-agnostic columns. Integrates normalization into listing loop to populate vmCount and networkCount for each provider item.

Sequence Diagram

sequenceDiagram
    participant List as List Flow
    participant Norm as normalizeProviderInventory
    participant GetProv as getSpecificProvider
    participant Count as countProviderResources
    participant Client as Provider Client

    List->>List: Iterate provider items
    List->>Norm: Normalize inventory data
    Norm->>GetProv: Get provider by name
    GetProv-->>Norm: Provider details
    Norm->>Norm: Determine relevant fields (vmCount, networkCount)
    alt Missing field & baseURL available
        Norm->>Count: Count resources
        Count->>Client: Fetch inventory items
        Client-->>Count: Items list
        Count->>Count: Count items (or -1 on error)
        Count-->>Norm: Count result
        Norm->>Norm: Store count in item
    else Missing field & no baseURL
        Norm->>Norm: Default count to 0 with logging
    end
    Norm-->>List: Normalized item
    List->>List: Add to output

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

• Area of focus: Logic in countProviderResources for error handling and resource counting (returns -1 on failure)
• Area of focus: Field mapping logic in getProviderRelevantFields to ensure correct resource-type associations
• Area of focus: Integration points in the listing loop where normalization is called and how missing data fallbacks are handled
• Area of focus: Provider retrieval logic in new helper methods (getProviders, getSpecificProvider) for consistency with existing patterns

Poem

🐰 Columns once scattered, now unified and bright,
Provider fields dance in normalized light,
From ad-hoc to elegant, counts flow just right,
Resource inventories—a refactored delight! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'update-provider-table' is vague and generic, using a broad term that doesn't convey the specific nature of the changes (provider inventory field normalization, dynamic column simplification, resource counting).	Consider a more specific title like 'Normalize provider inventory fields and simplify column generation' or 'Refactor provider table to use consistent field normalization' to better reflect the architectural changes.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch ec2-3

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

pkg/cmd/get/provider/list.go (3)

19-27: Unused providerType parameter.

The providerType parameter is never used in this function. If this is intentional for future extensibility, consider adding a comment or using _ string to make the intent explicit. Otherwise, remove it.

-func getProviderRelevantFields(providerType string) map[string]string {
+func getProviderRelevantFields(_ string) map[string]string {

Or remove the parameter entirely if getDynamicInventoryColumns also doesn't need provider-type-specific behavior.

---

66-74: Unused providerTypes parameter.

Similar to getProviderRelevantFields, the providerTypes parameter is unused. For consistency, either document the intent for future extensibility or remove both unused parameters.

-func getDynamicInventoryColumns(providerTypes map[string]bool) []output.Header {
+func getDynamicInventoryColumns(_ map[string]bool) []output.Header {

---

247-295: Consider reducing repetition in field extraction.

The 16+ sequential if ok blocks follow the same pattern. This could be simplified with a helper or loop:

// Define fields to extract
countFields := []string{
    "vmCount", "hostCount", "datacenterCount", "clusterCount",
    "networkCount", "datastoreCount", "storageClassCount", "product",
    "regionCount", "projectCount", "imageCount", "volumeCount",
    "volumeTypeCount", "diskCount", "storageCount", "storageDomainCount",
}

for _, field := range countFields {
    if val, ok := bulkProvider[field]; ok {
        item[field] = val
    }
}

This reduces code duplication and makes it easier to add new fields in the future.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 081e926 and fd196c6.

📒 Files selected for processing (1)

• pkg/cmd/get/provider/list.go (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/cmd/get/provider/list.go (2)

pkg/util/output/table.go (1)

• Header (65-68)

pkg/util/client/inventory.go (1)

• FetchProviderInventoryWithInsecure (45-88)

🔇 Additional comments (3)

pkg/cmd/get/provider/list.go (3)

29-64: LGTM!

The normalization logic is well-structured: it gracefully handles missing fields by attempting to count resources from the inventory API and falling back to 0 on failure. The debug logging is helpful for troubleshooting.

---

76-93: LGTM!

The resource counting logic is straightforward and handles errors appropriately. Returning -1 on failure allows the caller to distinguish between "0 resources" and "failed to count."

---

309-312: LGTM with a note on performance.

The normalization step is correctly placed after bulk data extraction. Since normalizeProviderInventory skips fields that already exist, it only makes additional HTTP requests when bulk data is unavailable or incomplete.

For large provider counts, if bulk fetch fails, this could result in 2 requests per provider. Verify this fallback behavior is acceptable for your deployment scenarios, or consider adding a flag to disable fallback counting.