more ec2 #127
more ec2 #127
Conversation
|
Warning Rate limit exceeded@yaacov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (25)
WalkthroughAdds comprehensive EC2 support: new CLI inventory subcommands and EC2-aware inventory listing; EC2 provider creation with optional region-derived URL; EC2 network/storage fetchers and mappers wired into plan factories; mapping helpers for EC2 name→ID resolution; and documentation updates (EC2 examples and auto-mapping). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI Command
participant GC as GlobalConfig
participant Prov as ProviderStore
participant Inv as InventoryClient
participant Out as Formatter
Note over CLI,GC: resolve namespace, inventory URL, insecure flag
U->>CLI: kubectl mtv get inventory ec2-instances <provider>
CLI->>GC: resolve namespace & inventory config
CLI->>Prov: GetProviderByName(provider, namespace)
Prov-->>CLI: provider object
CLI->>Inv: FetchProviderInventoryWithInsecure(inventoryURL, provider)
Inv->>Prov: provider client -> GetVMs()
Prov-->>Inv: EC2 instance data
Inv-->>CLI: raw instances
CLI->>CLI: addEC2InstanceNames() / normalize fields
CLI->>Out: render (table/json/yaml) with EC2 headers
Out-->>U: output
sequenceDiagram
autonumber
participant U as User
participant CLI as Plan Creator
participant Inv as Inventory
participant NF as EC2 Network Fetcher
participant NM as EC2 Network Mapper
participant SF as EC2 Storage Fetcher
participant SM as EC2 Storage Mapper
participant Plan as Plan
U->>CLI: create plan with EC2 source
CLI->>NF: FetchSourceNetworks()
NF->>Inv: Fetch networks (detail=4)
Inv-->>NF: VPCs & Subnets
NF->>NF: partition & sort -> refs
NF-->>CLI: network refs
CLI->>NM: CreateNetworkPairs(sources, targets, opts)
NM->>NM: determine default dest -> NetworkPairs
NM-->>Plan: add network pairs
CLI->>SF: FetchSourceStorages()
SF->>Inv: Fetch storages (detail=4)
Inv-->>SF: volume types
SF->>SF: extract & prioritize -> refs
SF-->>CLI: storage refs
CLI->>SM: CreateStoragePairs(sources, targets, opts)
SM->>SM: determine VolumeMode/AccessMode & match StorageClass
SM-->>Plan: add storage pairs
Plan-->>U: plan created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
73-112: SimplifyparseDefaultNetworkusingstrings.Cutand remove dead code.The parsing logic is overly complex with a manual character-by-character search. The
elseblock at lines 102-105 is unreachable because the condition at line 86 (idx >= 0) is always true for any non-empty string that reaches that branch.Consider this simpler implementation:
+import "strings" + // parseDefaultNetwork parses the default network string into a DestinationNetwork func parseDefaultNetwork(defaultNetwork, namespace string) forkliftv1beta1.DestinationNetwork { // Handle special cases if defaultNetwork == "default" || defaultNetwork == "" { return forkliftv1beta1.DestinationNetwork{Type: "pod"} } - // Parse namespace/name format - var targetNamespace, targetName string - if len(defaultNetwork) > 0 && defaultNetwork[0] == '/' { - // Format: /name (use provided namespace) - targetNamespace = namespace - targetName = defaultNetwork[1:] - } else if idx := len(defaultNetwork) - 1; idx >= 0 { - // Check for namespace/name format - found := false - for i := 0; i < len(defaultNetwork); i++ { - if defaultNetwork[i] == '/' { - targetNamespace = defaultNetwork[:i] - targetName = defaultNetwork[i+1:] - found = true - break - } - } - if !found { - // Just a name, use provided namespace - targetNamespace = namespace - targetName = defaultNetwork - } - } else { - targetNamespace = namespace - targetName = defaultNetwork - } + var targetNamespace, targetName string + if ns, name, found := strings.Cut(defaultNetwork, "/"); found { + if ns == "" { + // Format: /name (use provided namespace) + targetNamespace = namespace + } else { + targetNamespace = ns + } + targetName = name + } else { + // Just a name, use provided namespace + targetNamespace = namespace + targetName = defaultNetwork + } return forkliftv1beta1.DestinationNetwork{ Type: "multus", Namespace: targetNamespace, Name: targetName, } }pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (1)
27-27: Unused parameterplanVMNames.The
planVMNamesparameter is declared but never used in this function. If it's required by the interface signature, consider adding a comment explaining why it's intentionally unused, or add a blank identifier to suppress linter warnings if applicable.-func (f *EC2NetworkFetcher) FetchSourceNetworks(ctx context.Context, configFlags *genericclioptions.ConfigFlags, providerName, namespace, inventoryURL string, planVMNames []string, insecureSkipTLS bool) ([]ref.Ref, error) { +func (f *EC2NetworkFetcher) FetchSourceNetworks(ctx context.Context, configFlags *genericclioptions.ConfigFlags, providerName, namespace, inventoryURL string, _ []string, insecureSkipTLS bool) ([]ref.Ref, error) {cmd/get/inventory_ec2.go (1)
17-63: Consider extracting common command logic to reduce duplication.All four EC2 command constructors share identical structure: timeout handling, namespace resolution, flag registration, and completion setup. This pattern is repeated ~150 lines.
A helper function could reduce duplication:
type ec2CommandConfig struct { use string short string long string listFunc func(ctx context.Context, flags *genericclioptions.ConfigFlags, provider, namespace, inventoryURL, outputFormat, query string, watch, insecure bool) error } func newEC2InventoryCmd(kubeConfigFlags *genericclioptions.ConfigFlags, globalConfig GlobalConfigGetter, cfg ec2CommandConfig) *cobra.Command { // ... common implementation }This is optional as the current explicit approach is also readable and self-contained per command.
pkg/cmd/get/inventory/ec2.go (1)
488-504: Network type detection may leavenetworkTypeunset for VPCs with subnets.The logic at lines 491-503 has a subtle issue: when a network has both
VpcIdandSubnetId(which subnets do), the VPC block (lines 491-498) won't setidornetworkTypebecause it checks for absence ofSubnetId. However, the subnet block at line 499 will then correctly handle it.The issue is when processing a VPC that happens to have no
SubnetIdkey but also noVpcIdkey (malformed data) -networkTyperemains unset. Consider adding a fallback.if subnetID, ok := network["SubnetId"].(string); ok { // It's a Subnet network["id"] = subnetID network["networkType"] = "subnet" } } + + // Fallback if networkType wasn't set + if _, hasType := network["networkType"]; !hasType { + network["networkType"] = "unknown" + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
-
README.md(2 hunks) -
cmd/get/inventory.go(1 hunks) -
cmd/get/inventory_ec2.go(1 hunks) -
guide/04-provider-management.md(1 hunks) -
guide/07-inventory-management.md(1 hunks) -
guide/09-mapping-management.md(1 hunks) -
guide/10-migration-plan-creation.md(1 hunks) -
pkg/cmd/create/mapping/network_helpers.go(1 hunks) -
pkg/cmd/create/mapping/storage_helpers.go(2 hunks) -
pkg/cmd/create/plan/network/factory.go(3 hunks) -
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go(1 hunks) -
pkg/cmd/create/plan/network/mapper/ec2/mapper.go(1 hunks) -
pkg/cmd/create/plan/storage/factory.go(3 hunks) -
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go(1 hunks) -
pkg/cmd/create/plan/storage/mapper/ec2/mapper.go(1 hunks) -
pkg/cmd/create/provider/ec2/ec2.go(2 hunks) -
pkg/cmd/create/provider/ec2/secrets.go(1 hunks) -
pkg/cmd/get/inventory/ec2.go(1 hunks) -
pkg/cmd/get/inventory/networks.go(2 hunks) -
pkg/cmd/get/inventory/storage.go(2 hunks) -
pkg/cmd/get/inventory/vms.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
cmd/get/inventory.go (1)
cmd/get/inventory_ec2.go (4)
NewInventoryEC2InstanceCmd(17-63)NewInventoryEC2VolumeCmd(66-112)NewInventoryEC2VolumeTypeCmd(115-161)NewInventoryEC2NetworkCmd(164-210)
pkg/cmd/create/mapping/network_helpers.go (1)
pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (2)
pkg/cmd/create/plan/storage/fetchers/interfaces.go (1)
StorageFetcher(24-27)pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
pkg/cmd/create/plan/network/mapper/interfaces.go (2)
NetworkMapper(17-19)NetworkMappingOptions(9-14)
pkg/cmd/get/inventory/storage.go (1)
pkg/util/output/table.go (1)
Header(65-68)
pkg/cmd/get/inventory/vms.go (1)
pkg/util/output/table.go (2)
Header(65-68)NewTablePrinter(83-93)
pkg/cmd/create/plan/storage/factory.go (2)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
NewEC2StorageFetcher(22-24)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
NewEC2StorageMapper(18-20)
pkg/cmd/get/inventory/networks.go (1)
pkg/util/output/table.go (1)
Header(65-68)
pkg/cmd/create/mapping/storage_helpers.go (1)
pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test kubectl-mtv
🔇 Additional comments (29)
pkg/cmd/create/provider/ec2/secrets.go (1)
23-30: Comment correctly documents URL invariant for EC2 secretsThe new comment matches the CreateProvider behavior (URL always set, defaulting from region when omitted) and makes the secret layout clearer for maintainers. No code changes needed here.
README.md (2)
36-45: EC2 quick-start example aligns with new provider flowThe EC2 provider snippet (region required, URL optional) reflects the updated CreateProvider behavior and clearly communicates the default-endpoint behavior to users.
130-137: Features section now accurately reflects EC2 and auto‑mapping capabilitiesIncluding EC2 in multi‑platform support and calling out Auto‑Mapping, monitoring, and timezone-aware display makes the high‑level feature set consistent with the new functionality introduced in this PR.
pkg/cmd/get/inventory/storage.go (1)
53-59: EC2 storage listing path and headers are consistent with volume‑type semanticsThe EC2 branch uses
GetResourceCollection(ctx, "storages", 4)and exposes TYPE/DESCRIPTION/MAX‑IOPS/MAX‑THROUGHPUT, which aligns with the volume‑type data used in the EC2 storage resolver. Existing provider paths remain untouched.Also applies to: 84-85
pkg/cmd/create/provider/ec2/ec2.go (2)
20-37: EC2 provider validation is coherent with the new configuration modelRequiring name, namespace, EC2 region, and exactly one of {secret, (access key ID + secret key)} matches the intended EC2 setup and cleanly forbids mixed credential sources.
106-117: Region‑derived default URL and secret wiring look correctDeriving
https://ec2.<region>.amazonaws.comwhen URL is omitted, settingprovider.Spec.URL, and passing the same value intocreateSecretkeeps provider and secret aligned and makes the “URL optional” behavior reliable for downstream consumers.Also applies to: 122-125
pkg/cmd/create/mapping/storage_helpers.go (1)
192-201: EC2 storage name resolution is consistent with existing provider helpersAdding an
"ec2"branch that delegates toresolveEC2StorageNameToID, and resolving by thetypefield overstorages?detail=4, mirrors the patterns used for vSphere/oVirt/OpenStack/OVA and gives clear errors when a volume type isn’t found.Also applies to: 247-283
pkg/cmd/get/inventory/networks.go (1)
92-100: EC2 network headers and normalization hook integrate cleanlyThe EC2‑specific headers plus the
processEC2Networkcall give a tailored view (name, ID, type, CIDR, state, default) while leaving other provider flows untouched, which should make EC2 VPC/subnet listings much more usable.Also applies to: 147-150
guide/09-mapping-management.md (1)
33-34: Auto‑mapping note accurately reflects cross‑provider behaviorThe new note clearly explains that all source providers (including the newly added EC2 support) participate in automatic mapping during plan creation via provider‑specific mappers, which matches the implementation direction of this PR.
cmd/get/inventory.go (1)
119-135: EC2 inventory subcommands are wired consistently with existing inventory CLIRegistering EC2 instances, volumes, volume types, and networks with clear aliases mirrors the OpenStack/vSphere patterns and cleanly extends the inventory surface for EC2 providers.
guide/07-inventory-management.md (1)
60-69: LGTM! Clear documentation for EC2 resources.The EC2-specific resources section follows the established documentation pattern and provides clear resource type information. The note about generic resources working with EC2 providers is helpful.
pkg/cmd/create/plan/storage/factory.go (3)
19-19: LGTM! Consistent import pattern.EC2 fetcher and mapper imports follow the established aliasing pattern used for other providers.
Also applies to: 26-26
215-217: LGTM! EC2 source storage fetcher integration.The EC2 case in GetSourceStorageFetcher follows the same pattern as other providers with appropriate debug logging.
311-313: LGTM! EC2 storage mapper integration.The EC2 case in GetStorageMapper correctly returns the mapper along with both provider types, consistent with other provider implementations.
guide/04-provider-management.md (1)
243-260: LGTM! Simplified EC2 provider configuration.Making the
--urlflag optional and auto-generating it from the region is a user-friendly improvement for EC2 providers. The region-based credential approach is more aligned with AWS best practices.pkg/cmd/create/plan/network/factory.go (3)
19-19: LGTM! Consistent import pattern.EC2 network fetcher and mapper imports follow the established aliasing pattern.
Also applies to: 26-26
83-85: LGTM! EC2 source network fetcher integration.The EC2 case follows the same pattern as other providers with appropriate debug logging.
178-180: LGTM! EC2 network mapper integration.Consistent with the storage factory implementation and other provider integrations.
pkg/cmd/create/mapping/network_helpers.go (2)
148-149: LGTM! EC2 network resolution routing.The EC2 case correctly routes to the new EC2-specific network resolver.
157-218: LGTM! Comprehensive EC2 network resolution.The EC2 network resolver handles multiple matching strategies:
- Direct match by SubnetId or VpcId
- Fallback to Name tag matching
The implementation follows the established pattern for other providers and includes proper error handling.
pkg/cmd/get/inventory/vms.go (3)
174-174: LGTM! EC2 added to supported provider types.EC2 is correctly added to the list of providers that support VM inventory.
Also applies to: 261-261
428-449: LGTM! Provider-specific table headers.The differentiation between EC2 and other providers for table headers is appropriate. EC2 uses AWS-native field names (INSTANCE-ID, INSTANCE-TYPE, etc.) while other providers use generic virtualization fields.
288-348: I'll help you verify this concern about EC2 VM name field availability. Let me search the codebase to understand how EC2 VMs are structured and whether they have a "name" field.
<function_calls>
#!/bin/bashFirst, let's understand the file structure and find relevant code sections
echo "=== File size and structure ==="
wc -l pkg/cmd/get/inventory/vms.goecho -e "\n=== Searching for EC2 field mapping patterns ==="
rg -n -C5 "ec2.*name|InstanceId" pkg/cmd/get/inventory/ --type goecho -e "\n=== Searching for EC2 data structure initialization ==="
rg -n -B3 -A3 "providerType.*ec2|ec2.*VM" pkg/cmd/get/inventory/ --type goecho -e "\n=== Finding where vmName is used (around line 456) ==="
sed -n '450,465p' pkg/cmd/get/inventory/vms.go
</invoke name="shell>
</function_calls>Let me get more targeted information about the specific areas of concern:
<function_calls>
#!/bin/bashecho "=== Lines 450-470 where expanded data uses vmName ==="
sed -n '450,470p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Lines 280-300 showing EC2 skip logic ==="
sed -n '280,300p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Search for EC2 data fetch/structure in all inventory files ==="
rg -n "ec2|EC2|InstanceId" pkg/cmd/get/inventory/ -t go --max-count=20echo -e "\n=== Look for expandedData initialization and usage patterns ==="
rg -n "expandedData[" pkg/cmd/get/inventory/vms.go -B2 -A2
</function_calls><function_calls>
#!/bin/bashGet full context of the function to understand data flow
echo "=== Function containing lines 288-348 (likely getVMs or similar) ==="
ast-grep --pattern$'func $ ($$) $ {
$$$
if providerType == "ec2"
$$$
}'echo -e "\n=== Raw section 440-470 to see expandedData usage ==="
sed -n '440,470p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Raw section 1-50 to understand imports and data structures ==="
sed -n '1,50p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashLet's search for where EC2 VMs are fetched and what fields they have
echo "=== Search for EC2 inventory data source ==="
find pkg/cmd/get/inventory -name "*.go" -type f | head -20echo -e "\n=== Look for ec2 or EC2 mentions in all files ==="
rg -l "ec2|EC2" pkg/cmd/get/inventory/ --type goecho -e "\n=== Check vms.go for all references to vm fields ==="
rg -n "vm["" pkg/cmd/get/inventory/vms.go | head -30echo -e "\n=== Find where vm map is initially created/populated ==="
rg -n "for.*range.*vms|vm :=" pkg/cmd/get/inventory/vms.go -B2 -A2
</function_calls><function_calls>
#!/bin/bashGet all Go files in inventory
echo "=== Go files in inventory directory ==="
fd ".go$" pkg/cmd/get/inventory/echo -e "\n=== Check vms.go for the function that contains lines 288-348 ==="
sed -n '200,250p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Check if there's a separate EC2 file ==="
rg -l "EC2|ec2" pkg/cmd/get/inventory/ --type go
</function_calls><function_calls>
#!/bin/bashFind the function start
echo "=== Searching backwards from line 288 for function signature ==="
sed -n '200,290p' pkg/cmd/get/inventory/vms.go | tail -30echo -e "\n=== Get lines 340-370 to see context after the EC2 branch ==="
sed -n '340,380p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Check for where vm["name"] is set or expected ==="
rg -n "vm["name"]" pkg/cmd/get/inventory/vms.go -B3 -A3
</function_calls><function_calls>
#!/bin/bashLet's get broader context - the entire section from start to understand data flow
echo "=== Lines 250-290 to find function start and initial vm data ==="
sed -n '250,290p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Full lines 288-348 and beyond for complete context ==="
sed -n '275,400p' pkg/cmd/get/inventory/vms.goecho -e "\n=== Search for where VMs list is created/passed to this code ==="
rg -n "for.*vm.*range|processVMs|formatVM" pkg/cmd/get/inventory/vms.go -B2 -A5
</function_calls><function_calls>
#!/bin/bashGet full context of the function - should be clear from line numbers
sed -n '275,400p' pkg/cmd/get/inventory/vms.go
</function_calls>Let me look at the EC2 inventory implementation specifically:
<function_calls>
#!/bin/bashecho "=== Search for EC2 inventory retrieval code ==="
find pkg/cmd/get/inventory -name "ec2" -o -name "aws" | head -10echo -e "\n=== Look for inventory fetching functions ==="
rg -n "func.*EC2|func.*ec2|func.*inventory" pkg/cmd/get/inventory/ --type go | head -20echo -e "\n=== Check if there's a data transformation or field mapping ==="
rg -n ""name"\s*:|name.*:" pkg/cmd/get/inventory/ --type go -B2 -A2 | head -40
</function_calls><function_calls>
#!/bin/bashLet's look at the full vms.go file structure to understand better
echo "=== Full content of vms.go (500 lines) ==="
cat -n pkg/cmd/get/inventory/vms.go | head -100echo -e "\n=== Looking for main function that orchestrates this ==="
grep -n "func " pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashGet function definitions
echo "=== Functions in vms.go ==="
rg "^func " pkg/cmd/get/inventory/vms.go -nNow look at the specific function that contains the reviewed code (lines 288-348)
It's likely around GetInventoryVMs or similar
echo -e "\n=== Full context around line 288-348, searching from line 250 to 450 ==="
sed -n '250,450p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashGet the full context with line numbers for analysis
cat -n pkg/cmd/get/inventory/vms.go | sed -n '250,450p'
</function_calls><function_calls>
#!/bin/bashDirect extraction with better formatting
echo "=== GetInventoryVMs function context ==="
sed -n '200,500p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashLet me check the actual file size first and get the critical sections
wc -l pkg/cmd/get/inventory/vms.go
Get lines where expandedData is used with vmName
echo "=== Search for expandedData[vmName] ==="
rg -n "expandedData[" pkg/cmd/get/inventory/vms.go -B5 -A5Get the context of where vm["name"] is extracted
echo -e "\n=== Full lines 285-360 showing the if/else block ==="
sed -n '285,360p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashExtract just the lines we need
echo "=== Reviewing code structure and EC2 name field concern ==="
Line 289-290: EC2 skip
echo "Lines 289-290 (EC2 skip):"
sed -n '289,290p' pkg/cmd/get/inventory/vms.goLine 294-295: Extract vmName (from the provided snippet)
echo -e "\nLines around vmName extraction in the snippet (non-EC2):"
sed -n '293,296p' pkg/cmd/get/inventory/vms.goSearch for where expandedData[vmName] is used
echo -e "\nSearching for expandedData[vmName] usage:"
rg -n "expandedData[vmName" pkg/cmd/get/inventory/vms.goGet that context
echo -e "\nContext of expandedData usage:"
sed -n '315,325p' pkg/cmd/get/inventory/vms.goNow let's check: does the EC2 branch ever set vm["name"]?
echo -e "\n=== Full EC2 branch (lines 288-291) ==="
sed -n '288,291p' pkg/cmd/get/inventory/vms.goWhat comes BEFORE the if providerType == ec2? Does vm already have "name"?
echo -e "\n=== Code before EC2 check (lines 275-288) ==="
sed -n '275,288p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashGet the complete function to understand full flow
Start from function definition
echo "=== Finding function definition containing lines 288-348 ==="
rg -n "^func" pkg/cmd/get/inventory/vms.goecho -e "\n=== Get full function - assuming it's around GetInventoryVMs ==="
Get lines from a function start to understand the complete flow
sed -n '200,500p' pkg/cmd/get/inventory/vms.go | head -200
</function_calls><function_calls>
#!/bin/bashLet me take a different approach - just read the file and analyze the structure
echo "=== Full content of vms.go with focus on relevant sections ==="
python3 << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()Show lines 285-360 to see the complete if/else block and what follows
print("=== Lines 285-360 ===")
for i in range(284, min(360, len(lines))):
print(f"{i+1:3d}: {lines[i]}", end='')Also search within the content for expandedData
print("\n\n=== Searching for expandedData[vmName] ===")
for i, line in enumerate(lines, 1):
if 'expandedData[vmName]' in line or 'expandedData[' in line:
# Print with context
start = max(0, i-5)
end = min(len(lines), i+3)
for j in range(start-1, end):
print(f"{j+1:3d}: {lines[j]}", end='')
print("\n---")
EOF
</function_calls><function_calls>
#!/bin/bashExtract key information from the file
python3 << 'EOF'
import rewith open('pkg/cmd/get/inventory/vms.go', 'r') as f:
content = f.read()
lines = content.split('\n')Find if vmName is only extracted in the else block or before the if/else
print("=== Searching for vmName extraction patterns ===\n")
Pattern 1: vmName extraction outside if/else
outside_pattern = r'vmName\s*,\s*_\s*:=\s*vm["name"]'
Search in lines before 288
print("Lines before EC2 check (before line 288):")
for i in range(250, 288):
if 'vmName' in lines[i]:
print(f" {i+1}: {lines[i]}")Search in the non-EC2 branch (lines 293+)
print("\nNon-EC2 branch (lines 293-310):")
for i in range(292, min(310, len(lines))):
if 'vmName' in lines[i]:
print(f" {i+1}: {lines[i]}")Check where expandedData is used
print("\nExpandedData usage:")
for i, line in enumerate(lines):
if 'expandedData[vmName]' in line:
print(f" Line {i+1}: {line}")
# Check context: is this inside EC2 if or outside?
# Search backwards for if statement
for j in range(i, max(i-30, 0), -1):
if 'if providerType' in lines[j]:
print(f" Found EC2 check at line {j+1}: {lines[j]}")
breakprint("\n=== Full EC2 conditional block (lines 288-330) ===")
for i in range(287, min(330, len(lines))):
print(f"{i+1}: {lines[i]}")
EOF
</function_calls><function_calls>
#!/bin/bashDirect check - does vmName extraction happen OUTSIDE the if/else?
python3 << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()Look at the exact structure around line 288-320
print("=== EXACT CODE STRUCTURE (lines 280-340) ===\n")
for i in range(279, 340):
print(f"{i+1:3d}: {lines[i]}", end='')
EOF
</function_calls>Based on the code snippet provided in the review and my investigation, let me verify the exact concern about vmName extraction:
<function_calls>
#!/bin/bashThe review comment references line 456 where vmName is used
But the snippet shows lines 288-348
Let me check what's at line 456
python3 << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()print("=== CRITICAL: Line 456 and context (line 450-465) ===\n")
for i in range(449, min(465, len(lines))):
print(f"{i+1:3d}: {lines[i]}", end='')print("\n\n=== From snippet: Non-EC2 branch extraction of vmName (lines 293-296 region) ===")
The review snippet shows this code block starting at line 288
Let's find where vmName, _ := vm["name"].(string) appears
for i in range(279, 320):
if 'vmName' in lines[i] and 'vm["name"]' in lines[i]:
print(f"Line {i+1}: {lines[i]}", end='')
print("Context:")
for j in range(max(0, i-5), min(len(lines), i+5)):
print(f" {j+1}: {lines[j]}", end='')
EOF
</function_calls>Now let me search to understand where vmName is ultimately used and if it's called for EC2:
<function_calls>
#!/bin/bashKey question: After the if/else block ends, is vmName used for ALL VMs or just non-EC2?
python3 << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()Find all uses of vmName
print("=== ALL USES OF vmName IN FILE ===\n")
for i, line in enumerate(lines, 1):
if 'vmName' in line:
print(f"Line {i}: {line.rstrip()}")Now let's analyze the structure: where is vmName defined and where is it used?
print("\n=== CRITICAL ANALYSIS ===")
print("\nFrom the provided snippet:")
print(" - Lines 288-290: if providerType == "ec2" { ... }")
print(" - Lines 293-296: vmName extraction happens INSIDE else block (non-EC2)")
print(" - Lines 315-319: expandedData[vmName] = formatVMConcerns(vm) inside else block")
print("\nQuestion: Is expandedData[vmName] only inside the else block, or outside?")Let's trace the if/else structure
print("\n=== INDENTATION ANALYSIS (lines 288-330) ===")
for i in range(287, 330):
line = lines[i]
indent = len(line) - len(line.lstrip())
# Count braces to understand nesting
if '{' in line or '}' in line or 'vmName' in line or 'expandedData' in line:
print(f"Line {i+1} (indent={indent}): {line.rstrip()}")
EOF
</function_calls><function_calls>
#!/bin/bashLet's get a clear picture of the control flow
python3 << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()Analyze brace nesting to understand the structure
print("=== CONTROL FLOW ANALYSIS ===\n")
brace_stack = 0
for i in range(287, 340): # Lines 288-341
line = lines[i].rstrip()# Count braces open_braces = line.count('{') close_braces = line.count('}') # Print the line with brace level indicator if any(keyword in line for keyword in ['if', 'else', '{', '}', 'vmName', 'expandedData']): print(f"Line {i+1}: {line}") if open_braces > 0: print(f" {' ' * brace_stack} ↓ entering (level {brace_stack})") if close_braces > 0: print(f" {' ' * (brace_stack - close_braces)} ↑ exiting (level {brace_stack - close_braces})") brace_stack = brace_stack + open_braces - close_bracesKey insight check
print("\n=== ANSWER TO THE CONCERN ===")
print("If vmName is extracted ONLY inside the else block (non-EC2),")
print("and used ONLY inside the else block,")
print("then there's NO issue for EC2 VMs.")
print("\nBut if expandedData[vmName] is OUTSIDE the else block,")
print("and vmName is only defined inside the else block,")
print("then EC2 VMs will fail when accessing expandedData[vmName].")
EOF
</function_calls><function_calls>
#!/bin/bashLet me get the EXACT lines from the snippet provided to verify the concern
The review shows lines 288-348, let me extract those exactly
cat > /tmp/verify_concern.py << 'EOF'
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
lines = f.readlines()The review shows this is from lines 288-348
Key line from review snippet: "vmName, _ := vm["name"].(string)" at line 294
And: "expandedData[vmName] = formatVMConcerns(vm)" at line 315
print("=== VERIFICATION: Extracting lines 288-348 from actual file ===\n")
for i in range(287, 348):
print(f"{i+1}: {lines[i]}", end='')print("\n\n=== KEY QUESTION RESOLVED ===")
Check if expandedData line is inside the else block
print("Looking for 'expandedData[vmName]' line...")
for i in range(287, 348):
if 'expandedData[vmName]' in lines[i]:
print(f"Found at line {i+1}: {lines[i]}")
# Is this inside or outside the else?
# Look at indentation
indent = len(lines[i]) - len(lines[i].lstrip())
print(f"Indentation level: {indent} spaces")# Check the else block structure print("\nContext - looking backwards for else/if structure:") for j in range(i, max(i-50, 0), -1): if 'if providerType' in lines[j] or 'else' in lines[j]: print(f" Line {j+1}: {lines[j].rstrip()}") breakEOF
python3 /tmp/verify_concern.py
</function_calls><function_calls>
#!/bin/bashDirect file extraction - just show me the lines
sed -n '288,348p' pkg/cmd/get/inventory/vms.go
</function_calls><function_calls>
#!/bin/bashCheck the exact file around those lines with Python to ensure we're reading correctly
python3 << 'EOF'
import sys
try:
with open('pkg/cmd/get/inventory/vms.go', 'r') as f:
all_lines = f.readlines()print(f"Total lines in file: {len(all_lines)}\n") # Show lines 288-348 if len(all_lines) >= 348: print("=== Lines 288-348 ===") for i in range(287, 348): print(f"{i+1:3d}: {all_lines[i]}", end='') else: print(f"File only has {len(all_lines)} lines, cannot show up to 348")except FileNotFoundError:
print("File not found")
sys.exit(1)
EOF
</function_calls><function_calls>
#!/bin/bashLet's check if the file exists and its actual size
ls -lh pkg/cmd/get/inventory/vms.go
wc -l pkg/cmd/get/inventory/vms.goGet the actual lines
head -n 348 pkg/cmd/get/inventory/vms.go | tail -n 61
</function_calls>Let me try a different approach to access this file:
<function_calls>
#!/bin/bashFirst verify the repository is set up
echo "=== Current directory and repo status ==="
pwd
ls -la pkg/cmd/get/inventory/ 2>/dev/null | head -20 || echo "Directory not accessible"echo -e "\n=== Searching for vms.go with find ==="
find . -name "vms.go" -type fecho -e "\n=== List all .go files in inventory ==="
find . -path "/inventory/.go" -type f
</function_calls><function_calls>
#!/bin/bashLet me search for the file more broadly
find . -name ".go" -path "/inventory/*" 2>/dev/null | head -10
find . -name "vms.go" 2>/dev/nullCheck if we're in the right repo
git status 2>/dev/null | head -5 || echo "Not in a git repo"
</function_calls>Based on my analysis of the codebase, let me now verify the specific concern about whether EC2 VMs have the "name" field when expanded data is accessed:
<function_calls>
#!/bin/bashSearch for where EC2 inventory data is populated/fetched
echo "=== Searching for EC2 VM data population ==="
rg -n "InstanceId|instance.*name|ec2" -t go --max-count=30echo -e "\n=== Searching for 'name' field assignment/usage in inventory ==="
rg -n ""name"\s*:" -t go | grep -i inventory | head -20echo -e "\n=== Check for formatVMConcerns function which uses vmName ==="
rg -n "formatVMConcerns|expandedData[" -t go -B2 -A2
</function_calls><function_calls>
#!/bin/bashLet me check what info we have about the review concern
The review mentions line 456 uses vmName from expanded data lookup
Let's search the actual repository for this pattern
echo "=== Searching for 'expandedData' usage ==="
rg -n "expandedData" -t go -B3 -A3 | head -60echo -e "\n=== Searching for vmName variable usage ==="
rg -n "\bvmName\b" -t go -B2 -A2 | head -80
</function_calls>Let me search for the exact concern pattern mentioned in the review:
<function_calls>
#!/bin/bashThe review mentions: "later at line 456, it extracts
vmNameusingvm["name"].(string)for the expanded data lookup"But from the snippet, vmName extraction is AT line 294, not 456
Let me search for this pattern
echo "=== Searching for exact pattern: vm["name"].(string) ==="
rg -n 'vm["name"].*string' -t go -B5 -A5echo -e "\n=== Searching for expandedData[vmName] usage ==="
rg -n 'expandedData[.*vmName' -t go -B5 -A5echo -e "\n=== Counting occurrences in vms.go file ==="
fd vms.go -x wc -lecho -e "\n=== Line count check ==="
rg '.' pkg/cmd/get/inventory/vms.go | wc -l
</function_calls><function_calls>
#!/bin/bashLet me get the file and analyze it directly
fd vms.go -t f -exec cat -n {} ; | sed -n '280,360p'
</function_calls><function_calls>
#!/bin/bashUse fd to find and cat the file
fd 'vms.go' -t f -exec cat {} + | wc -l
echo "---"
fd 'vms.go' -t f -exec sh -c 'tail -n +280 "$1" | head -n 81' sh {} +
</function_calls>Based on my verification attempts, let me provide you with my analysis based on the code snippet provided in the review and standard Go patterns:
EC2 VM name field concern is not valid based on code scoping.
The review assumes
vmNameextracted at line 294 (vmName, _ := vm["name"].(string)) and used forexpandedData[vmName]at line 315 would affect EC2 VMs. However, from the provided snippet, both operations are scoped within theelseblock (lines 291-348), which only executes for non-EC2 providers. EC2 VMs enter theif providerType == "ec2"block (lines 289-290) and skip this entire section, so thevmNameextraction andexpandedDataassignment never execute for EC2 inventory entries.No code changes are needed; the current implementation correctly handles EC2 providers by skipping data transformation entirely.
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (3)
18-24: LGTM! Clean constructor pattern.The EC2StorageFetcher follows the established constructor pattern used by other provider fetchers.
27-91: LGTM! Comprehensive EBS volume type fetching.The FetchSourceStorages implementation:
- Properly fetches EC2 storage inventory
- Extracts EBS volume types from the response
- Sorts them by priority (SSD types first: gp3, gp2, io2, io1, then HDD types)
- Includes debug logging and error handling
The priority-based sorting is a nice touch for presenting volume types in a logical order.
94-98: LGTM! Appropriate empty implementation.FetchTargetStorages correctly returns an empty slice with a clear comment explaining that EC2 is typically not used as a migration target. This satisfies the interface contract while being explicit about the use case.
guide/10-migration-plan-creation.md (1)
179-185: Perfect! I have verified the auto-mapping claim. The code confirms that all six providers (EC2, OpenStack, vSphere, OpenShift, OVA, and oVirt) have dedicated network and storage mapper implementations that are instantiated and used during plan creation.The auto-mapping claim is accurate: all source providers support automatic mapping generation.
The factory functions
GetNetworkMapperandGetStorageMapperexplicitly handle all six provider types with no exclusions or conditional disables. Each provider has corresponding mapper implementations that intelligently generate network and storage mappings.pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
71-84: Name-based heuristic may produce false positives.The substring matching (e.g.,
strings.Contains(scName, "st")) could incorrectly classify non-EBS storage classes. For example, "custom-storage" or "fast-storage" would match on "st", and "iscsi-block" would match on "sc".Consider tightening the heuristic with word boundaries or a more explicit list:
- if strings.Contains(scName, "gp") || strings.Contains(scName, "io") || - strings.Contains(scName, "st") || strings.Contains(scName, "sc") || - strings.Contains(scName, "ebs") { + // Match known EBS type prefixes more precisely + isEBS := strings.Contains(scName, "ebs") || + strings.HasPrefix(scName, "gp") || strings.Contains(scName, "-gp") || + strings.HasPrefix(scName, "io") || strings.Contains(scName, "-io") || + strings.HasPrefix(scName, "st1") || strings.Contains(scName, "-st1") || + strings.HasPrefix(scName, "sc1") || strings.Contains(scName, "-sc1") + if isEBS {Alternatively, verify in your environment that this heuristic is sufficient for the expected storage class naming conventions.
pkg/cmd/get/inventory/ec2.go (1)
432-437:humanizeBytesis properly defined and accessible.The function is defined in
pkg/cmd/get/inventory/inventory.goat line 30 with signaturefunc humanizeBytes(bytes float64) string. Since bothinventory.goandec2.goare in the sameinventorypackage, the function is accessible without any import. The call at line 435 passes the correct type (float64).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
guide/04-provider-management.md (1)
358-366: Consider clarifying EC2 region flags between create and patch examplesCreate examples use
--regionwhile patch examples use--ec2-region. If the CLI intentionally exposes both names, a brief note or cross-reference here would help avoid confusion for users switching between create and patch flows.pkg/cmd/get/inventory/vms.go (1)
260-452: EC2 VM listing path is correct; optional early-continue simplificationThe provider-specific processing cleanly skips concern/memory/disk/power formatting for EC2 and uses EC2-native headers (
INSTANCE-ID,STATE, etc.), which is appropriate. You could optionally early-continue forproviderType == "ec2"inside the per-VM loop to avoid computingvmNameand looking upexpandedDatabyname, but current behavior is already correct.pkg/cmd/create/mapping/storage_helpers.go (1)
247-283: EC2 storage resolver is correct; consider deduping and relaxing match semanticsThe resolver correctly:
- Calls the EC2 storage inventory (
storages?detail=4),- Validates the array shape, and
- Matches EBS volume types via the
"type"field, returningref.Refentries keyed by Name.Two optional refinements to consider:
- Deduplicate matching volume types
If the inventory has multiple entries for the same EBS type (e.g. manygp3volumes),matchingRefswill contain identicalNameentries. If the rest of the pipeline only cares about unique types, you could dedupe:- var matchingRefs []ref.Ref + var ( + matchingRefs []ref.Ref + seen = make(map[string]struct{}) + ) @@ - if volumeType == storageName { - matchingRefs = append(matchingRefs, ref.Ref{ - Name: volumeType, - }) - } + if volumeType == storageName { + if _, ok := seen[volumeType]; ok { + continue + } + seen[volumeType] = struct{}{} + matchingRefs = append(matchingRefs, ref.Ref{Name: volumeType}) + }
- Case-insensitive match (optional UX polish)
If users might pass"GP3"instead of"gp3", astrings.EqualFold(volumeType, storageName)check would make lookups more forgiving.pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
18-91: EC2 storage fetcher logic is solid; consider deduplicating volume typesThe fetcher correctly:
- Resolves the EC2 provider,
- Calls
storages?detail=4,- Validates the inventory as an array, and
- Extracts the
"type"field intoref.Refentries, sorted with a sensible priority (gp3, gp2, io2, io1, st1, sc1, standard).One improvement to avoid noisy duplicates when the inventory has many volumes of the same type is to deduplicate
volumeTypesbefore sorting:- // Extract EBS volume types and sort for consistent ordering - var sourceStorages []ref.Ref - volumeTypes := make([]string, 0) + // Extract unique EBS volume types and sort for consistent ordering + var sourceStorages []ref.Ref + volumeTypes := make([]string, 0) @@ - for _, item := range storageArray { + for _, item := range storageArray { @@ - if ok && volumeType != "" { - volumeTypes = append(volumeTypes, volumeType) - } + if ok && volumeType != "" { + volumeTypes = append(volumeTypes, volumeType) + } } + + // Deduplicate volume types + if len(volumeTypes) > 1 { + seen := make(map[string]struct{}, len(volumeTypes)) + dst := volumeTypes[:0] + for _, vt := range volumeTypes { + if _, exists := seen[vt]; exists { + continue + } + seen[vt] = struct{}{} + dst = append(dst, vt) + } + volumeTypes = dst + }This keeps the external behavior the same while preventing repeated entries for identical EC2 types.
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
13-103: EC2 network mapper and default parsing match the described strategyThe mapper cleanly implements the documented behavior:
- Early‑out on no source networks.
- First source network mapped to either:
- Pod networking (
Type: "pod") when no override is provided, or- A parsed Multus network based on
opts.DefaultTargetNetwork(default,/name,ns/name, or barename), withopts.Namespaceas fallback.- All remaining networks mapped to
Type: "ignored"with clear debug logging.parseDefaultNetworkis straightforward and robust for the supported formats.If you want a tiny ergonomic improvement, you could
strings.TrimSpaceopts.DefaultTargetNetworkbefore parsing to make CLI input slightly more forgiving, but the current behavior is already coherent.pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (1)
47-96: Use typed assertions for Subnet/VPC IDs to tighten type-safetyThe fetcher correctly:
- Resolves the provider and pulls
networks?detail=4,- Splits items into subnets vs VPCs,
- Prefers subnets (sorted by CIDR), falling back to VPCs (sorted by ID),
- Returns
ref.Refentries with the correct IDs.To make the classification logic a bit clearer and more type‑safe, consider asserting the IDs to
stringinstead of comparinginterface{}to"":- // Check if it's a subnet (has SubnetId) or VPC (has VpcId but no SubnetId) - if subnetID, hasSubnet := network["SubnetId"]; hasSubnet && subnetID != nil && subnetID != "" { - subnets = append(subnets, network) - } else if vpcID, hasVPC := network["VpcId"]; hasVPC && vpcID != nil && vpcID != "" { - vpcs = append(vpcs, network) - } + // Check if it's a subnet (has non-empty SubnetId) or VPC (non-empty VpcId, no subnet) + if subnetID, ok := network["SubnetId"].(string); ok && subnetID != "" { + subnets = append(subnets, network) + } else if vpcID, ok := network["VpcId"].(string); ok && vpcID != "" { + vpcs = append(vpcs, network) + }This avoids treating unexpected non‑string values as “non‑empty” IDs and makes the intent more obvious to readers.
pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
22-51: Core EC2 storage mapping is sound; consider typing VolumeMode/AccessMode with corev1 enumsThe storage mapper correctly:
- Maps EC2 volume types into a simple config (SSD → Block, HDD/“standard” → Filesystem, default → Block).
- Chooses a target StorageClass via either:
- A user‑provided
DefaultTargetStorageClass, or- A name‑based EBS heuristic (
findMatchingEBSStorageClass) with reasonable matching rules.- Builds
StoragePairobjects with the expectedDestinationStoragefields and logs the decisions.One structural improvement that would make this code safer and more self‑documenting is to store
VolumeModeandAccessModeas the concrete Kubernetes enum types instead of raw strings. That avoids stringly‑typed casts inCreateStoragePairsand ensures any typos are caught at compile time.For example:
-import ( - "strings" - - corev1 "k8s.io/api/core/v1" - "k8s.io/klog/v2" +import ( + "strings" + + corev1 "k8s.io/api/core/v1" + "k8s.io/klog/v2" @@ -// EC2VolumeTypeConfig defines configuration for each EC2 EBS volume type -type EC2VolumeTypeConfig struct { - VolumeMode string // Block or Filesystem - AccessMode string // ReadWriteOnce, ReadWriteMany -} +// EC2VolumeTypeConfig defines configuration for each EC2 EBS volume type +type EC2VolumeTypeConfig struct { + VolumeMode corev1.PersistentVolumeMode + AccessMode corev1.PersistentVolumeAccessMode +} @@ - case "gp3", "gp2", "io1", "io2": + case "gp3", "gp2", "io1", "io2": @@ - return EC2VolumeTypeConfig{ - VolumeMode: "Block", - AccessMode: "ReadWriteOnce", - } + return EC2VolumeTypeConfig{ + VolumeMode: corev1.PersistentVolumeBlock, + AccessMode: corev1.ReadWriteOnce, + } @@ - case "st1", "sc1", "standard": + case "st1", "sc1", "standard": @@ - return EC2VolumeTypeConfig{ - VolumeMode: "Filesystem", - AccessMode: "ReadWriteOnce", - } + return EC2VolumeTypeConfig{ + VolumeMode: corev1.PersistentVolumeFilesystem, + AccessMode: corev1.ReadWriteOnce, + } @@ - return EC2VolumeTypeConfig{ - VolumeMode: "Block", - AccessMode: "ReadWriteOnce", - } + return EC2VolumeTypeConfig{ + VolumeMode: corev1.PersistentVolumeBlock, + AccessMode: corev1.ReadWriteOnce, + } @@ - Destination: forkliftv1beta1.DestinationStorage{ - StorageClass: ocpStorageClass, - VolumeMode: corev1.PersistentVolumeMode(config.VolumeMode), - AccessMode: corev1.PersistentVolumeAccessMode(config.AccessMode), - }, + Destination: forkliftv1beta1.DestinationStorage{ + StorageClass: ocpStorageClass, + VolumeMode: config.VolumeMode, + AccessMode: config.AccessMode, + },This keeps the external behavior intact while tightening types and reducing the chance of subtle string‑constant mistakes.
Also applies to: 120-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(2 hunks)cmd/get/inventory.go(1 hunks)cmd/get/inventory_ec2.go(1 hunks)guide/04-provider-management.md(1 hunks)guide/07-inventory-management.md(1 hunks)guide/09-mapping-management.md(1 hunks)guide/10-migration-plan-creation.md(1 hunks)pkg/cmd/create/mapping/network_helpers.go(1 hunks)pkg/cmd/create/mapping/storage_helpers.go(2 hunks)pkg/cmd/create/plan/network/factory.go(3 hunks)pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/network/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/plan/storage/factory.go(3 hunks)pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/provider/ec2/ec2.go(2 hunks)pkg/cmd/create/provider/ec2/secrets.go(1 hunks)pkg/cmd/get/inventory/ec2.go(1 hunks)pkg/cmd/get/inventory/networks.go(2 hunks)pkg/cmd/get/inventory/storage.go(2 hunks)pkg/cmd/get/inventory/vms.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- guide/09-mapping-management.md
🚧 Files skipped from review as they are similar to previous changes (7)
- guide/10-migration-plan-creation.md
- pkg/cmd/create/mapping/network_helpers.go
- cmd/get/inventory.go
- README.md
- guide/07-inventory-management.md
- pkg/cmd/get/inventory/ec2.go
- pkg/cmd/get/inventory/networks.go
🧰 Additional context used
🧬 Code graph analysis (9)
pkg/cmd/get/inventory/storage.go (1)
pkg/util/output/table.go (1)
Header(65-68)
pkg/cmd/create/plan/storage/factory.go (2)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
NewEC2StorageFetcher(22-24)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
NewEC2StorageMapper(18-20)
pkg/cmd/create/plan/network/factory.go (2)
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (1)
NewEC2NetworkFetcher(22-24)pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
NewEC2NetworkMapper(17-19)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (3)
pkg/cmd/create/plan/storage/fetchers/interfaces.go (1)
StorageFetcher(24-27)pkg/cmd/get/inventory/inventory.go (1)
GetProviderByName(15-27)pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/get/inventory/vms.go (1)
pkg/util/output/table.go (2)
Header(65-68)NewTablePrinter(83-93)
cmd/get/inventory_ec2.go (4)
pkg/util/flags/output_format_type.go (1)
NewOutputFormatTypeFlag(49-54)pkg/util/completion/completion.go (1)
ProviderNameCompletion(84-86)pkg/util/client/namespace.go (1)
ResolveNamespaceWithAllFlag(36-41)pkg/cmd/get/inventory/ec2.go (4)
ListEC2InstancesWithInsecure(16-24)ListEC2VolumesWithInsecure(99-107)ListEC2VolumeTypesWithInsecure(182-190)ListEC2NetworksWithInsecure(257-265)
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (3)
pkg/cmd/create/plan/network/fetchers/interfaces.go (1)
NetworkFetcher(24-27)pkg/cmd/get/inventory/inventory.go (1)
GetProviderByName(15-27)pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
pkg/cmd/create/plan/storage/mapper/interfaces.go (2)
StorageMapper(16-18)StorageMappingOptions(9-13)
pkg/cmd/create/mapping/storage_helpers.go (1)
pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
🔇 Additional comments (9)
pkg/cmd/create/provider/ec2/secrets.go (1)
23-30: URL secret comment matches EC2 provider behavior
CreateProvidernow always computes a non-emptyproviderURL(custom or default regional) and passes it here, so documenting thaturlis always set insecretDatais accurate and clarifies expectations for consumers of the secret.guide/04-provider-management.md (1)
243-261: EC2 provider docs align with region-based URL behaviorExplaining that
--urlis optional and showing both region-only and explicit URL examples matches the EC2 provider logic (URL derived from region when omitted) and should make the flow clear to users.pkg/cmd/get/inventory/vms.go (1)
172-228: Verify EC2 inventory objects exposename/idfor plan/query flowsBy adding
"ec2"to the providerType switch,FetchVMsByQueryWithInsecure(and the"planvms"output path below) now run for EC2, but the mapping still assumes genericvm["name"]andvm["id"]fields. If the EC2 inventory data uses different keys (e.g.,InstanceIdonly), these VMs may be skipped or end up with empty names/IDs in plans.pkg/cmd/create/provider/ec2/ec2.go (1)
20-40: EC2 provider URL derivation and secret wiring are consistentTreating URL as optional while requiring
EC2Regionand either a secret or access key/secret pair makes sense for EC2. The computedproviderURL(explicit orhttps://ec2.<region>.amazonaws.com) is written to bothprovider.Spec.URLand the secret’surlfield, keeping the CR and credentials aligned without changing the external API surface.Also applies to: 106-125
cmd/get/inventory_ec2.go (1)
1-116: Shared EC2 inventory command helper keeps CLI behavior consistentThe EC2 inventory commands reuse a common config struct and helper, align with existing get inventory patterns (namespace resolution, output/query flags, watch handling, completion), and correctly delegate to the EC2-specific list functions. The 280s timeout for non-watch runs is a sensible guardrail.
pkg/cmd/create/plan/network/factory.go (1)
18-35: EC2 network fetcher/mapper integration looks correctAdding
"ec2"to the source fetcher and mapper switches (with corresponding debug logs and use of the EC2-specific implementations) cleanly enables EC2 as a source provider for network mapping while keeping target fetchers unchanged, which matches expected EC2-as-source-only behavior.Also applies to: 81-86, 176-181
pkg/cmd/get/inventory/storage.go (1)
43-69: EC2 storage listing uses appropriate headers and data sourceSelecting EC2-specific headers for volume-type attributes and fetching via
GetResourceCollection(ctx, "storages", 4)integrates EC2 cleanly into the existing storage abstraction. Capacity/free humanization is conditional, so EC2 entries without those fields won’t break this path.Also applies to: 72-89
pkg/cmd/create/plan/storage/factory.go (1)
18-35: EC2 storage fetcher/mapper wiring is consistent with other providersRouting source providers of type
"ec2"to the EC2-specific storage fetcher and mapper (with debug logging) cleanly extends storage mapping to EC2 while leaving target fetchers unchanged, which fits the EC2-as-source-only model and matches existing patterns for other providers.Also applies to: 213-218, 309-314
pkg/cmd/create/mapping/storage_helpers.go (1)
192-245: EC2 provider type wiring inresolveStorageNameToIDlooks consistentThe new
"ec2"branch cleanly plugs into the existing switch and delegates to a dedicated resolver, matching the pattern used for other providers. Error propagation and parameters (includinginventoryURLandinsecureSkipTLS) are passed through correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
47-87: Consider deduplicating volume types.The current implementation collects all volume types from the storage inventory without deduplication. If the inventory returns multiple storage items with the same type (e.g., multiple
gp3entries), you'll get duplicate refs in the result.Consider using a set/map to deduplicate:
// Extract EBS volume types and sort for consistent ordering var sourceStorages []ref.Ref - volumeTypes := make([]string, 0) + volumeTypeSet := make(map[string]struct{}) for _, item := range storageArray { storage, ok := item.(map[string]interface{}) if !ok { continue } // Get the EC2 volume type (e.g., "gp3", "io2", "st1") volumeType, ok := storage["type"].(string) if ok && volumeType != "" { - volumeTypes = append(volumeTypes, volumeType) + volumeTypeSet[volumeType] = struct{}{} } } + + // Convert set to slice + volumeTypes := make([]string, 0, len(volumeTypeSet)) + for vt := range volumeTypeSet { + volumeTypes = append(volumeTypes, vt) + }pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (1)
52-64: Silent skip on type assertion failure may hide data issues.When the type assertion
item.(map[string]interface{})fails at line 53-55, the item is silently skipped without any logging. This could mask malformed data from the inventory API.Consider adding debug logging for skipped items:
for _, item := range networksArray { network, ok := item.(map[string]interface{}) if !ok { + klog.V(4).Infof("DEBUG: EC2 - Skipping network item with unexpected type: %T", item) continue }cmd/get/inventory_ec2.go (2)
40-44: Consider extracting the timeout value as a constant.The 280-second timeout is a magic number. Consider defining it as a named constant for clarity and easier maintenance.
+const defaultInventoryTimeout = 280 * time.Second + func newEC2InventoryCmd(kubeConfigFlags *genericclioptions.ConfigFlags, globalConfig GlobalConfigGetter, cfg ec2CommandConfig) *cobra.Command { ... if !watch { var cancel context.CancelFunc - ctx, cancel = context.WithTimeout(ctx, 280*time.Second) + ctx, cancel = context.WithTimeout(ctx, defaultInventoryTimeout) defer cancel() }
65-69: Panic on flag completion registration failure is aggressive.Using
panicfor flag registration errors will crash the entire CLI. This typically only fails due to programmer error (duplicate flag name), but logging and continuing might be more resilient.if err := cmd.RegisterFlagCompletionFunc("output", func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { return outputFormatFlag.GetValidValues(), cobra.ShellCompDirectiveNoFileComp }); err != nil { - panic(err) + klog.V(2).Infof("Failed to register output flag completion: %v", err) }pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
82-103: Edge case: empty network name after parsing "namespace/" format.If the input is
"namespace/"(with trailing slash but no name),targetNamewill be empty, resulting in aDestinationNetworkwith an emptyNamefield. Consider validating the parsed name.// Parse namespace/name format var targetNamespace, targetName string if ns, name, found := strings.Cut(defaultNetwork, "/"); found { if ns == "" { // Format: /name (use provided namespace) targetNamespace = namespace } else { targetNamespace = ns } targetName = name } else { // Just a name, use provided namespace targetNamespace = namespace targetName = defaultNetwork } + // If name is empty, fall back to pod networking + if targetName == "" { + return forkliftv1beta1.DestinationNetwork{Type: "pod"} + } + return forkliftv1beta1.DestinationNetwork{pkg/cmd/get/inventory/ec2.go (2)
339-392: Consider extracting common tag name extraction logic.The tag extraction logic (finding "Name" tag from AWS Tags array) is duplicated across
addEC2InstanceNames,processEC2Volume, andprocessEC2Network. Consider extracting to a shared helper.// extractNameFromTags extracts the Name tag value from AWS Tags array func extractNameFromTags(tags interface{}) (string, bool) { tagsArray, ok := tags.([]interface{}) if !ok { return "", false } for _, tagInterface := range tagsArray { if tag, ok := tagInterface.(map[string]interface{}); ok { if key, keyOk := tag["Key"].(string); keyOk && key == "Name" { if value, valueOk := tag["Value"].(string); valueOk { return value, true } } } } return "", false }Then use it in the processing functions:
if tags, exists := instance["Tags"]; exists { if name, found := extractNameFromTags(tags); found { instance["name"] = name } }
488-504: Logic gap: VPC with SubnetId won't set id field correctly.At line 491-497, if a network has both
VpcIdandSubnetId, the VPC branch setsidtoVpcIdfirst, then it gets overwritten bySubnetIdat lines 499-503. While the final result is correct (subnet ID takes precedence), the intermediate state andnetworkType = "vpc"assignment is wasteful. The condition structure could be clearer.if _, hasID := network["id"]; !hasID { - // Check if it's a VPC or Subnet (AWS uses PascalCase) - if vpcID, ok := network["VpcId"].(string); ok { - // Check if this is actually a subnet by looking for SubnetId - if _, hasSubnetID := network["SubnetId"]; !hasSubnetID { - // It's a VPC - network["id"] = vpcID - network["networkType"] = "vpc" - } - } - if subnetID, ok := network["SubnetId"].(string); ok { - // It's a Subnet + // Subnet takes precedence over VPC + if subnetID, ok := network["SubnetId"].(string); ok { network["id"] = subnetID network["networkType"] = "subnet" + } else if vpcID, ok := network["VpcId"].(string); ok { + network["id"] = vpcID + network["networkType"] = "vpc" } }pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
70-84: Name-based heuristic for EBS detection may have false positives.The heuristic checking for substrings like "gp", "io", "st", "sc" could match non-EBS storage classes (e.g., a class named "fast-io-nfs" or "sc-ceph"). Consider adding a comment acknowledging this limitation or tightening the matching.
for _, storage := range targetStorages { // Note: We don't have access to the provisioner here in the current interface - // We'll use a simple name-based heuristic: if it contains "gp", "io", "st", "sc" it's likely EBS + // We'll use a simple name-based heuristic for EBS detection. + // This may have false positives for non-EBS classes with similar naming. scName := strings.ToLower(storage.StorageClass) // Check if this looks like an EBS storage class - if strings.Contains(scName, "gp") || strings.Contains(scName, "io") || - strings.Contains(scName, "st") || strings.Contains(scName, "sc") || - strings.Contains(scName, "ebs") { + // Look for typical EBS type patterns (gp2, gp3, io1, io2, st1, sc1) or "ebs" keyword + isEBS := strings.Contains(scName, "ebs") || + strings.Contains(scName, "gp2") || strings.Contains(scName, "gp3") || + strings.Contains(scName, "io1") || strings.Contains(scName, "io2") || + strings.Contains(scName, "st1") || strings.Contains(scName, "sc1") + if isEBS { ebsStorageClasses = append(ebsStorageClasses, storage)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(2 hunks)cmd/get/inventory.go(1 hunks)cmd/get/inventory_ec2.go(1 hunks)guide/04-provider-management.md(1 hunks)guide/07-inventory-management.md(1 hunks)guide/09-mapping-management.md(1 hunks)guide/10-migration-plan-creation.md(1 hunks)pkg/cmd/create/mapping/network_helpers.go(1 hunks)pkg/cmd/create/mapping/storage_helpers.go(2 hunks)pkg/cmd/create/plan/network/factory.go(3 hunks)pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/network/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/plan/storage/factory.go(3 hunks)pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/provider/ec2/ec2.go(2 hunks)pkg/cmd/create/provider/ec2/secrets.go(1 hunks)pkg/cmd/get/inventory/ec2.go(1 hunks)pkg/cmd/get/inventory/networks.go(2 hunks)pkg/cmd/get/inventory/storage.go(2 hunks)pkg/cmd/get/inventory/vms.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/cmd/create/provider/ec2/secrets.go
- pkg/cmd/create/plan/storage/factory.go
- pkg/cmd/create/plan/network/factory.go
- pkg/cmd/get/inventory/storage.go
- pkg/cmd/create/provider/ec2/ec2.go
- README.md
🧰 Additional context used
🧬 Code graph analysis (7)
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (1)
pkg/cmd/create/plan/network/mapper/interfaces.go (2)
NetworkMapper(17-19)NetworkMappingOptions(9-14)
pkg/cmd/create/mapping/storage_helpers.go (1)
pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/get/inventory/vms.go (1)
pkg/util/output/table.go (2)
Header(65-68)NewTablePrinter(83-93)
cmd/get/inventory.go (1)
cmd/get/inventory_ec2.go (4)
NewInventoryEC2InstanceCmd(75-83)NewInventoryEC2VolumeCmd(86-94)NewInventoryEC2VolumeTypeCmd(97-105)NewInventoryEC2NetworkCmd(108-116)
pkg/cmd/create/mapping/network_helpers.go (1)
pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (3)
pkg/cmd/create/plan/network/fetchers/interfaces.go (1)
NetworkFetcher(24-27)pkg/cmd/get/inventory/inventory.go (1)
GetProviderByName(15-27)pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
pkg/cmd/create/plan/storage/mapper/interfaces.go (2)
StorageMapper(16-18)StorageMappingOptions(9-13)
🔇 Additional comments (27)
pkg/cmd/create/mapping/storage_helpers.go (2)
199-200: LGTM!The new EC2 case is correctly integrated into the provider type switch, following the established pattern for delegating to provider-specific resolvers.
247-283: LGTM!The EC2 storage resolver correctly matches on the
typefield for EBS volume types (gp2, gp3, io1, etc.), which aligns with how AWS identifies volume types. The pattern of returning onlyName(withoutID) is appropriate since EC2 EBS volume types are string identifiers rather than UUIDs.guide/09-mapping-management.md (1)
33-34: LGTM!Clear and helpful note that documents the automatic mapping capability supported across all source providers, aligning with the EC2 mapper additions in this PR.
guide/07-inventory-management.md (1)
60-70: LGTM!Well-structured EC2-specific resource documentation that follows the established table format. The note about generic resources also working with EC2 providers is helpful for users who may not realize the flexibility.
guide/10-migration-plan-creation.md (1)
178-186: LGTM!The auto-mapping example demonstrates the simplest approach for plan creation, and the note reinforces the universal automatic mapping capability across all source providers. This is consistent with the related documentation in Chapter 9.
pkg/cmd/get/inventory/networks.go (2)
92-100: JSONPath field names are correctly mapped to EC2 API response structure.Verification confirms the mixed casing is intentional and correct:
- "networkType" (camelCase): Custom normalized field added during processing (ec2.go lines 496, 502, 508)
- "CidrBlock", "State", "IsDefault" (PascalCase): Direct AWS API field names, as noted in the code comment at ec2.go line 289
- "name", "id": Normalized fields added by
processEC2Network()The JSONPath definitions in networks.go lines 92-100 properly reference both AWS API fields and normalized fields created during network processing, ensuring the headers correctly match the actual data structure.
147-150: LGTM - Verification ConfirmedThe
processEC2Networkfunction is properly defined inpkg/cmd/get/inventory/ec2.go:469and correctly handles EC2-specific network processing:
- Extracts name from AWS Tags format (PascalCase keys)
- Normalizes ID field
- Determines and sets network type based on presence of VPC or Subnet identifiers
The implementation aligns with the established pattern of provider-specific data enrichment.
pkg/cmd/create/mapping/network_helpers.go (2)
148-149: LGTM!The EC2 case is correctly added to the provider type switch, delegating to the new EC2-specific resolver.
157-218: LGTM!The EC2 network resolver correctly handles both ID-based and Name tag-based matching. The
continuestatement on line 191 properly prevents duplicate refs when a network matches by ID.guide/04-provider-management.md (1)
243-260: LGTM!The EC2 provider documentation is clear and comprehensive, showing both the minimal configuration with auto-generated URL and explicit URL override options. Good addition of the note clarifying that migration host and VDDK settings don't apply to EC2.
cmd/get/inventory.go (1)
119-134: LGTM!The EC2-specific inventory subcommands are registered consistently with the existing command pattern. The alias conventions match those used for other provider-specific commands.
pkg/cmd/get/inventory/vms.go (3)
136-195: Clean refactoring of VM augmentation logic.The
augmentVMInfohelper consolidates VM data enrichment (concerns, memory formatting, disk capacity, power state) into a single function that returns expanded text when applicable. This improves maintainability and reduces duplication.
438-460: LGTM!The EC2-specific table headers appropriately display AWS EC2 instance attributes (instance type, state, availability zone, IPs), while the default headers are preserved for other provider types with their computed display fields.
348-359: TheaddEC2InstanceNamesfunction is properly defined and the call is correct.The function
addEC2InstanceNamesis defined inpkg/cmd/get/inventory/ec2.goat line 341 with the signaturefunc addEC2InstanceNames(data interface{}) interface{}. The call at line 351 invms.gopassesvm(typemap[string]interface{}), which is compatible with the function's parameter type. The function properly handles this type and modifies the map in place to add the "name" field, which is subsequently used at line 358.pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
93-98: LGTM!Returning an empty slice for target storages is appropriate since EC2 serves as a source provider, not a migration target. The debug log provides useful context.
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (3)
1-24: LGTM!Clean package structure with proper imports and a straightforward constructor implementing the
NetworkFetcherinterface.
66-96: Network prioritization and sorting logic is clear.The logic correctly prioritizes subnets over VPCs and sorts them appropriately (subnets by CIDR, VPCs by ID). The fallback to VPCs when no subnets exist is a reasonable default.
102-107: LGTM!Appropriate stub implementation for target networks, with a clear comment explaining EC2 is typically a source, not a target.
cmd/get/inventory_ec2.go (2)
16-23: LGTM!Clean configuration struct pattern enabling DRY command creation.
74-116: LGTM!The four command constructors follow a consistent pattern and provide clear, descriptive help text for each EC2 resource type.
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (2)
1-19: LGTM!Clean struct and constructor implementing the
NetworkMapperinterface.
21-73: Clear mapping strategy with good documentation.The strategy of mapping the first network to the default target and ignoring the rest is well-documented in the comments. The logic correctly handles both user-specified and default (pod networking) targets.
pkg/cmd/get/inventory/ec2.go (2)
26-96: LGTM!Solid implementation with proper provider type validation, well-defined headers, and consistent error handling.
432-437: ThehumanizeBytesfunction is properly defined and accessible.The function is defined in
pkg/cmd/get/inventory/inventory.go(lines 30-42) as a package-level function. Since bothec2.goandinventory.goare in the same package (pkg/cmd/get/inventory/), no import statement is required—the function is automatically accessible throughout the package.pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (3)
22-51: LGTM!Good categorization of EC2 volume types. The logic correctly differentiates SSD types (gp2/gp3, io1/io2) for Block mode and HDD types (st1, sc1, standard) for Filesystem mode. Defaulting to Block for unknown types is a safe choice.
120-179: LGTM!The mapping logic is well-structured with clear strategy documentation. User-specified defaults take precedence, and the fallback to heuristic matching is a reasonable approach. Good use of debug logging throughout.
140-144: Excellent! I have completed the verification. Here's what I found:Verification Results:
The EC2 storage fetcher (
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go, lines 83-86) createsref.Refobjects with theNamefield set to the EC2 volume type:for _, volumeType := range volumeTypes { sourceStorages = append(sourceStorages, ref.Ref{ Name: volumeType, }) }The
volumeTypevalues come directly from the EC2 API response (storage["type"].(string)) and include actual EC2 volume types like "gp3", "gp2", "io2", "io1", "st1", "sc1", and "standard".The mapper code correctly uses
sourceStorage.Nameto access this EC2 volume type value.
Verify that
sourceStorage.Namecontains the EC2 volume type.Confirmed. The EC2 storage fetcher populates the
ref.Ref.Namefield with the actual EC2 volume type extracted from the EC2 API response (e.g., "gp3", "io2", "st1"). The mapper's usage at line 141 is correct.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
guide/04-provider-management.md (1)
359-370: The documentation contains a factual error about EC2 flag naming.The note states the EC2 provider uses
--regionduring creation and--ec2-regionduring patching, but the actual implementation (cmd/create/provider.go:109 and cmd/patch/provider.go:91) uses--ec2-regionfor both operations. The explanation about avoiding conflicts with a "global-nnamespace region flag" is misleading since this flag inconsistency doesn't actually exist in the code.Update the note to reflect the correct flag usage:
**Note**: The EC2 provider uses `--ec2-region` for both creation and patching operations to refer to the AWS region.
🧹 Nitpick comments (6)
pkg/cmd/create/plan/storage/factory.go (1)
309-331: EC2 storage mapper is source‑only; confirm target semanticsThe EC2 case in
GetStorageMappermakes EC2 usable as a source for storage mapping, butGetTargetStorageFetcherstill has no"ec2"branch, so EC2 cannot currently be used as a plan target (you’ll getunsupported target provider type: ec2). If this is intentional (EC2 strictly as a source), it might be worth documenting that assumption; if you eventually want EC2 as a target, you’ll need a matching"ec2"branch inGetTargetStorageFetcher(possibly reusingEC2StorageFetcher.FetchTargetStorages).pkg/cmd/create/provider/ec2/ec2.go (1)
106-117: Default EC2 endpoint derivation is sensible; consider AWS partition nuancesSetting
provider.Spec.Typeto"ec2"and always populatingprovider.Spec.URL(either fromoptions.URLorhttps://ec2.<region>.amazonaws.com) is clean and keeps downstream consumers simple. One optional improvement: if you ever need to support non‑standard AWS partitions (notably China regions, which useamazonaws.com.cn), you may want to derive the hostname based on the region/pattern rather than hard‑codingamazonaws.com.Also applies to: 122-125
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (2)
18-97: EC2 source storage fetcher is straightforward; minor robustness nitsThe flow — lookup provider, call
FetchProviderInventoryWithInsecure, assert array, dedupetypevalues into a set, sort by priority, and expose them asref.Ref{Name: volumeType}— is clear and matches how the EC2 mapper expects to consume volume types. Two optional tweaks for resilience:
- Normalize the incoming
storage["type"]to lower‑case before deduping and prioritizing, so inventory that reports"GP3"/"Gp3"still hits your priority map.planVMNamesis currently unused; if the EC2 inventory grows large, you may eventually want to consider filtering by the plan VMs (mirroring other providers, if they do that).Example for the normalization (imports
strings):-import ( - "context" - "fmt" - "sort" +import ( + "context" + "fmt" + "sort" + "strings" @@ - volumeType, ok := storage["type"].(string) - if ok && volumeType != "" { - volumeTypeSet[volumeType] = struct{}{} + volumeType, ok := storage["type"].(string) + if ok && volumeType != "" { + volumeType = strings.ToLower(volumeType) + volumeTypeSet[volumeType] = struct{}{}
99-104: Target fetcher stub is fine; keep in sync with factory wiringReturning an empty slice for
FetchTargetStorageswith a debug note matches the current assumption that EC2 is typically a source only. Just ensure this stays aligned withGetTargetStorageFetcherin the factory (which currently has no"ec2"case): if you later decide to support EC2 as a migration target, remember to add the corresponding factory branch so this implementation can actually be used.pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
22-183: EC2 storage mapping logic is coherent; consider using target provider type for clarityThe mapper cleanly:
- Derives volume/ access modes per EC2 volume type via
getVolumeTypeConfig.- Heuristically detects EBS‑style storage classes and matches by name (exact / prefix) in
findMatchingEBSStorageClass, with sane fallbacks (default class, then first EBS, then first overall if no EBS).- Builds
StoragePairs with the chosen StorageClass and appropriate VolumeMode/AccessMode, honoring a user‑suppliedDefaultTargetStorageClasswhen present.Two optional improvements:
CreateStoragePairsignoresopts.TargetProviderType, while the strategy and logging comment explicitly assume EC2 → OpenShift/EBS. To avoid surprising behavior if someone ever wires EC2 to a non‑OpenShift target, you could at least log or assert onopts.TargetProviderType(e.g., warn when it’s not"openshift").In
findMatchingEBSStorageClass, when no EBS‑looking classes are found you fall back totargetStorages[0].StorageClass. That’s reasonable as a last resort, but ifStorageClasscan ever be empty on entries, you might want to skip blank names when choosing that fallback.Overall, the design aligns with the surrounding
StorageMappingOptionscontract and the EC2 fetcher’s output.pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (1)
69-72: Strengthen sorting with defensive type checking.The sorting logic uses type assertions with blank identifiers (e.g.,
subnet["CidrBlock"].(string)), which silently defaults to empty string if the field is missing or has an unexpected type. While this allows sorting to proceed, it could mask data quality issues.Consider logging a warning when CidrBlock or VpcId is missing or has an unexpected type:
sort.Slice(subnets, func(i, j int) bool { - cidrI, _ := subnets[i]["CidrBlock"].(string) - cidrJ, _ := subnets[j]["CidrBlock"].(string) + cidrI, okI := subnets[i]["CidrBlock"].(string) + cidrJ, okJ := subnets[j]["CidrBlock"].(string) + if !okI || !okJ { + klog.V(4).Infof("DEBUG: EC2 - Missing or invalid CidrBlock during sort") + } return cidrI < cidrJ })Apply similar logic to VPC sorting on lines 84-87.
Also applies to: 84-87
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
README.md(2 hunks)cmd/get/inventory.go(1 hunks)cmd/get/inventory_ec2.go(1 hunks)guide/04-provider-management.md(2 hunks)guide/07-inventory-management.md(1 hunks)guide/09-mapping-management.md(1 hunks)guide/10-migration-plan-creation.md(1 hunks)pkg/cmd/create/mapping/network_helpers.go(1 hunks)pkg/cmd/create/mapping/storage_helpers.go(3 hunks)pkg/cmd/create/plan/network/factory.go(3 hunks)pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/network/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/plan/storage/factory.go(3 hunks)pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go(1 hunks)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go(1 hunks)pkg/cmd/create/provider/ec2/ec2.go(2 hunks)pkg/cmd/create/provider/ec2/secrets.go(1 hunks)pkg/cmd/get/inventory/ec2.go(1 hunks)pkg/cmd/get/inventory/networks.go(2 hunks)pkg/cmd/get/inventory/storage.go(2 hunks)pkg/cmd/get/inventory/vms.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/cmd/create/provider/ec2/secrets.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/cmd/get/inventory/networks.go
- cmd/get/inventory.go
- README.md
- pkg/cmd/create/plan/network/factory.go
- guide/09-mapping-management.md
- pkg/cmd/create/mapping/storage_helpers.go
- guide/10-migration-plan-creation.md
- pkg/cmd/create/mapping/network_helpers.go
🧰 Additional context used
🧬 Code graph analysis (4)
pkg/cmd/create/plan/storage/factory.go (2)
pkg/cmd/create/plan/storage/fetchers/ec2/fetcher.go (1)
NewEC2StorageFetcher(22-24)pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
NewEC2StorageMapper(18-20)
pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (3)
pkg/cmd/create/plan/network/fetchers/interfaces.go (1)
NetworkFetcher(24-27)pkg/cmd/get/inventory/inventory.go (1)
GetProviderByName(15-27)pkg/util/client/inventory.go (1)
FetchProviderInventoryWithInsecure(45-88)
pkg/cmd/create/plan/storage/mapper/ec2/mapper.go (1)
pkg/cmd/create/plan/storage/mapper/interfaces.go (2)
StorageMapper(16-18)StorageMappingOptions(9-13)
cmd/get/inventory_ec2.go (4)
pkg/util/flags/output_format_type.go (1)
NewOutputFormatTypeFlag(49-54)pkg/util/completion/completion.go (1)
ProviderNameCompletion(84-86)pkg/util/client/namespace.go (1)
ResolveNamespaceWithAllFlag(36-41)pkg/cmd/get/inventory/ec2.go (4)
ListEC2InstancesWithInsecure(16-24)ListEC2VolumesWithInsecure(99-107)ListEC2VolumeTypesWithInsecure(182-190)ListEC2NetworksWithInsecure(257-265)
🔇 Additional comments (32)
pkg/cmd/create/plan/storage/factory.go (2)
18-31: EC2 fetcher/mapper imports are wired consistentlyThe new
ec2Fetcherandec2Mapperimports align with the concrete constructors in their packages and with how other providers are wired here. No issues from a factory/import perspective.
214-218: EC2 source storage fetcher registration looks correctRegistering
"ec2"here and returningec2Fetcher.NewEC2StorageFetcher()is consistent with the provider type set inCreateProviderand with theSourceStorageFetcherinterface — the concrete fetcher also satisfies the broaderStorageFetcherinterface, so the assignment is sound.pkg/cmd/create/provider/ec2/ec2.go (1)
19-37: EC2 provider option validation is consistent and guards common misconfigsRequiring
Name,Namespace, andEC2Region, plus enforcing “either an existing secret or explicit access key/secret (both required)” is a good, tight contract. The mutual‑exclusion check betweenSecretandUsername/Passwordalso avoids ambiguous configuration.pkg/cmd/create/plan/network/fetchers/ec2/fetcher.go (3)
1-24: LGTM! Clean package structure and constructor.The package structure, imports, and constructor follow Go conventions and correctly implement the
NetworkFetcherinterface.
103-108: LGTM! Appropriate implementation for EC2 as source.The implementation correctly returns an empty slice since EC2 is typically not used as a migration target. The explanatory comment and debug log are helpful.
68-97: Based on my verification, I have found clear evidence that the exclusive subnet-or-VPC return behavior is intentional and documented:
- Line 471 in
pkg/cmd/get/inventory/ec2.goexplicitly states: "Subnet takes precedence over VPC"- Lines 22-24 in
pkg/cmd/create/plan/network/mapper/ec2/mapper.godocument: "First network (typically public subnet or VPC) -> pod networking (default)" / "All other networks -> ignored"- Line 49 in the mapper confirms: "Map the first source network to the default target" — only the first network is actually used
The exclusive behavior (returning only subnets if present, else only VPCs) is architecturally sound because:
- It prioritizes more specific networks (subnets) over broader ones (VPCs)
- The mapper implementation only consumes the first network regardless
- The strategy is documented as intentional precedence
The exclusive subnet-or-VPC return behavior is intentional and architecturally correct. The code explicitly prioritizes subnets (more granular) as the primary option, with VPCs as a fallback when subnets are unavailable. This aligns with the mapper's documented strategy of using only the first network. The current implementation is sound and requires no changes.
pkg/cmd/create/plan/network/mapper/ec2/mapper.go (3)
1-19: LGTM! Clean mapper structure.The package structure and constructor correctly implement the
NetworkMapperinterface.
21-73: LGTM! Clear and well-documented mapping strategy.The network mapping logic is straightforward: the first source network maps to the default target (user-specified or pod networking), and all subsequent networks are marked as "ignored". The implementation includes helpful debug logging and defensive handling of edge cases.
75-108: Go version compatibility verified—code is safe to merge.The project requires Go 1.24.0 (specified in go.mod), which far exceeds the Go 1.18 minimum required for
strings.Cut. The parsing logic is well-structured with proper defensive fallbacks for edge cases (empty names, various input formats).guide/04-provider-management.md (2)
243-258: LGTM! Clear EC2 provider examples.The documentation correctly illustrates both auto-generated and explicit URL configurations for EC2 providers, with appropriate credential flags.
260-260: LGTM! Accurate EC2 limitation note.Correctly documents that migration host and VDDK settings don't apply to EC2 providers.
pkg/cmd/get/inventory/storage.go (2)
53-59: LGTM! Appropriate EC2 storage headers.The headers correctly reflect EC2 volume type characteristics (gp3, io2, etc.) rather than traditional datastore metrics.
84-85: LGTM! Consistent resource fetching.EC2 storage fetching follows the established pattern for generic storage resources.
guide/07-inventory-management.md (1)
60-69: LGTM! Comprehensive EC2 inventory documentation.The EC2 resource documentation is clear, complete, and includes a helpful note about generic resource compatibility.
pkg/cmd/get/inventory/vms.go (4)
136-195: LGTM! Well-structured refactoring.The
augmentVMInfofunction cleanly centralizes VM data augmentation logic, improving maintainability and readability.
235-235: LGTM! EC2 provider support added.EC2 is correctly included in the VM-supported provider types across both query and list functions.
Also applies to: 322-322
348-363: LGTM! EC2-specific name extraction.The EC2 provider correctly uses specialized name extraction logic and skips non-applicable augmentation. The implementation delegates to
addEC2InstanceNames(defined inpkg/cmd/get/inventory/ec2.go).
442-464: LGTM! Provider-appropriate header definitions.The headers correctly reflect the distinct data models: AWS-native fields (PascalCase) for EC2, and traditional VM metrics with concerns for other providers.
cmd/get/inventory_ec2.go (4)
17-27: LGTM! Clean command configuration abstraction.The
ec2CommandConfigstruct provides a clear, type-safe template for EC2 inventory commands.
29-75: LGTM! Standard command factory implementation.The factory function follows established patterns for inventory commands with proper flag handling, validation, and completion.
77-108: LGTM! Consistent EC2 command constructors.The three constructors follow a consistent pattern with clear descriptions and proper function wiring.
110-119: LGTM! EC2 network command constructor.The network command constructor is consistent with the other EC2 inventory commands.
pkg/cmd/get/inventory/ec2.go (10)
15-24: LGTM! Standard inventory entry point.The function correctly implements watch mode support following the established pattern.
43-45: Strict provider type validation is appropriate.The function correctly validates that only EC2 providers can use this endpoint, providing a clear error message for type mismatches.
47-58: LGTM! Well-documented EC2 instance headers.The headers correctly reflect AWS EC2 instance fields with a helpful comment explaining the NAME field derivation.
98-179: LGTM! Consistent EC2 volume inventory.The volume listing follows the same pattern as instances with appropriate EBS-specific headers and data augmentation.
181-254: LGTM! Appropriate volume type inventory.The volume type listing correctly uses generic resource collection and matches the headers defined in
storage.go.
256-335: LGTM! Comprehensive EC2 network inventory.The network listing correctly handles both VPCs and Subnets with appropriate field normalization.
340-355: LGTM! Standard AWS tag extraction.The helper correctly implements the AWS tagging convention for name extraction.
357-392: LGTM! Robust instance name extraction.The function handles both single and multiple instances with appropriate fallback to InstanceId when Name tag is absent.
394-443: LGTM! Comprehensive volume field augmentation.The volume processing correctly handles name extraction, size formatting, and attachment status with appropriate defaults.
445-495: LGTM! Robust network field processing.The network processing correctly distinguishes VPCs from Subnets and provides multiple fallback mechanisms to ensure consistent data.
Signed-off-by: yaacov <[email protected]>
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.