Sankofa/docs/reference/CODE_INCONSISTENCIES.md

# Code Inconsistencies Found

## Critical Inconsistencies

### 1. Missing Variable Assignment in Controller

**File**: `crossplane-provider-proxmox/pkg/controller/virtualmachine/controller.go:64`

**Issue**: Code search shows line 64 has `proxmox.NewClient(` but the actual file shows it's correctly assigned. However, there's a potential issue with error handling.

**Current Code**:
```go
// Line 63-72
// Create Proxmox client
proxmoxClient, err := proxmox.NewClient(
    site.Endpoint,
    creds.Username,
    creds.Password,
    site.InsecureSkipTLSVerify,
)
if err != nil {
    return ctrl.Result{}, errors.Wrap(err, "cannot create Proxmox client")
}
```

**Status**: ✅ Actually correct - variable is assigned. No fix needed.

### 2. Inconsistent Client Creation in VMScaleSet Controller

**File**: `crossplane-provider-proxmox/pkg/controller/vmscaleset/controller.go:47`

**Issue**: Creates client with empty parameters, likely a bug.

**Current Code**:
```go
proxmoxClient := proxmox.NewClient("", "", "")
```

**Problem**: This will always fail or use invalid credentials.

**Fix Required**: Should use proper credentials from ProviderConfig, similar to virtualmachine controller.

### 3. Inconsistent Error Handling Patterns

**Location**: Multiple files

**Pattern 1** (virtualmachine/controller.go):
- Credentials error → requeue after 30s
- Site error → requeue after 30s
- VM creation error → no requeue (immediate return)

**Pattern 2** (Other controllers):
- Various requeue strategies

**Recommendation**: Standardize error handling with:
- Retryable errors → requeue with exponential backoff
- Non-retryable errors → no requeue, update status with error condition
- Partial creation errors → cleanup + requeue with longer delay

### 4. importdisk API Usage Without Version Check

**File**: `crossplane-provider-proxmox/pkg/proxmox/client.go:397`

**Issue**: Uses importdisk API without checking if it's available.

**Current Code**:
```go
importPath := fmt.Sprintf("/nodes/%s/qemu/%d/importdisk", spec.Node, vmID)
if err := c.httpClient.Post(ctx, importPath, importConfig, &importResult); err != nil {
    return nil, errors.Wrapf(err, "failed to import image...")
}
```

**Problem**: No check if Proxmox version supports this API.

**Fix Required**: Add version check or API availability check before use.

### 5. Inconsistent Status Field Names

**File**: `crossplane-provider-proxmox/apis/v1alpha1/virtualmachine_types.go:56`

**Issue**: Status field is `VMID` but JSON tag is `vmId` (camelCase).

**Current Code**:
```go
VMID int `json:"vmId,omitempty"`
```

**Problem**: Inconsistent naming (Go uses VMID, JSON uses vmId).

**Impact**: Minor - works but inconsistent.

### 6. No Cleanup on Partial VM Creation

**File**: `crossplane-provider-proxmox/pkg/controller/virtualmachine/controller.go:142-145`

**Issue**: When CreateVM fails, no cleanup of partially created VM.

**Current Code**:
```go
createdVM, err := proxmoxClient.CreateVM(ctx, vmSpec)
if err != nil {
    return ctrl.Result{}, errors.Wrap(err, "cannot create VM")
}
```

**Problem**: If VM is created but import fails, VM remains orphaned.

**Fix Required**: Add cleanup logic in error path.

### 7. Lock File Handling Not Used in Error Recovery

**File**: `crossplane-provider-proxmox/pkg/proxmox/client.go:803-821`

**Issue**: UnlockVM function exists but not called during error recovery.

**Current Code**: UnlockVM is defined but never used in CreateVM error paths.

**Fix Required**: Call UnlockVM before DeleteVM in cleanup operations.

---

## Summary of Required Fixes

1. ✅ **virtualmachine/controller.go:64** - Actually correct, no fix needed
2. ❌ **vmscaleset/controller.go:47** - Fix client creation
3. ⚠️ **Error handling** - Standardize across all controllers
4. ❌ **importdisk API** - Add version/availability check
5. ⚠️ **Status field naming** - Consider consistency (low priority)
6. ❌ **Partial VM cleanup** - Add cleanup in error paths
7. ❌ **Lock file handling** - Use UnlockVM in cleanup

---

*Last Updated: 2025-12-12*