feat: implement instance types and resource limits#42
Conversation
📝 WalkthroughWalkthroughThis PR adds instance type support to the cloud platform, introducing a new configurable parameter for selecting instance specifications (CPU, memory, disk) when launching instances. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant Service as InstanceService
participant TypeRepo as InstanceTypeRepository
participant DB as Database
participant Backend as Compute Backend
Client->>Handler: POST /instances {name, image, ports, instance_type, vpc_id, ...}
Handler->>Service: LaunchInstance(ctx, name, image, ports, instanceType, vpcID, subnetID, volumes)
Service->>Service: Validate instanceType (default: "basic-2")
Service->>TypeRepo: GetByID(ctx, instanceType)
TypeRepo->>DB: SELECT * FROM instance_types WHERE id = ?
DB-->>TypeRepo: {vcpus: 2, memory_mb: 4096, disk_gb: 50, ...}
TypeRepo-->>Service: domain.InstanceType
Service->>Service: Create Instance with InstanceType field
Service->>Backend: CreateInstance(ctx, CreateInstanceOptions{CPULimit, MemoryLimit, DiskLimit, ...})
Backend-->>Service: instance created
Service-->>Handler: domain.Instance {id, name, instance_type, ...}
Handler-->>Client: 200 OK {data: {id, name, instance_type, ...}}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/database.md (1)
120-155:⚠️ Potential issue | 🟡 MinorUpdate the seeded instance types list to be complete.
The documentation is incomplete. The migration seeds seven instance types, but the doc only lists four examples:
basic-1,basic-2,standard-1, andperformance-2. Add the missing types—standard-2,standard-4, andperformance-1—to the documentation to prevent drift.The default value
basic-2is correct and matches the migration.internal/core/services/instance_test.go (1)
527-536:⚠️ Potential issue | 🟡 MinorComplete or remove the placeholder volume lookup test.
This test currently doesn’t exercise any behavior or make assertions, so it provides no coverage. Consider wiring it to
LaunchInstance(or remove it) and assert the expected repository calls.If you want, I can draft a focused test case that exercises volume lookup by ID and name.
🤖 Fix all issues with AI agents
In `@docs/api-reference.md`:
- Around line 176-189: The documented response for GET /instance-types is
missing fields present on the InstanceType model; update the example JSON and
schema to include network_mbps, price_per_hour, and category so it matches the
serialized API output. Locate the GET /instance-types section and the
InstanceType examples and add the network_mbps (number), price_per_hour
(number/string as used in serialization), and category (string) properties to
each instance object and any schema tables or descriptions referencing
InstanceType.
In `@internal/core/ports/instance_type.go`:
- Around line 9-15: The repository port InstanceTypeRepository is missing
Create, Update and Delete methods; add these CRUD methods keeping
context.Context as the first parameter and using domain.InstanceType types:
declare Create(ctx context.Context, it *domain.InstanceType)
(*domain.InstanceType, error), Update(ctx context.Context, it
*domain.InstanceType) (*domain.InstanceType, error) and Delete(ctx
context.Context, id string) error so the interface contains List, GetByID,
Create, Update and Delete.
In `@internal/core/services/instance_test.go`:
- Around line 55-66: The test uses a non-strict mock expectation for instance
type lookup (itRepo.On("GetByID", ...).Maybe()) so the new instance type
resolution can be skipped; remove .Maybe() on the GetByID expectation in
TestLaunchInstanceSuccess (and any similar LaunchInstance tests) so the
expectation is strict, and add an assertion for the mock (e.g., defer
itRepo.AssertExpectations(t)) alongside the existing repo.AssertExpectations(t)
to ensure GetByID was actually called.
In `@internal/core/services/instance.go`:
- Around line 169-175: The code silently leaves cpuLimit/memLimit/diskLimit at
zero when s.instanceTypeRepo.GetByID(ctx, inst.InstanceType) returns an error;
change this to handle it explicitly by checking if itErr != nil and
returning/propagating a clear error from the surrounding provisioning method (or
at minimum log a warning and apply safe fallback limits from config) instead of
continuing with zero values — update the GetByID error handling so cpuLimit,
memLimit, and diskLimit are only set when itErr == nil and otherwise
short-circuit with an error (or set documented fallback constants) to avoid
unintended unlimited/default backend limits.
In `@internal/handlers/instance_handler.go`:
- Around line 41-47: The InstanceType field in the request struct (InstanceType
string) is optional but its JSON tag lacks omitempty; update the struct tag for
InstanceType (the field named InstanceType in the request struct in
internal/handlers/instance_handler.go) to include `omitempty` in the json tag
(e.g., `json:"instance_type,omitempty"`) so optional fields follow the project's
convention.
In `@internal/repositories/postgres/instance_type_repo.go`:
- Around line 35-43: The iteration over rows in the function that builds the
types slice (using rows.Next() and r.scanInstanceType) is missing a
post-iteration check for rows.Err(); after the for loop, call rows.Err() and if
non-nil return nil and that error (or wrap it) so any iteration errors (e.g.,
connection issues) are propagated instead of silently ignored.
In `@pkg/sdk/compute.go`:
- Around line 50-59: The Instance struct is missing the InstanceType field, so
although LaunchInstance sends "instance_type" consumers won't see it in
responses; add a string field named InstanceType to the Instance struct with the
JSON tag `json:"instance_type,omitempty"` (preserve existing field
ordering/formatting with other fields) and ensure any related
marshaling/unmarshaling expectations remain unchanged so listing/getting
instances surface the instance type.
🧹 Nitpick comments (13)
internal/core/domain/autoscaling.go (1)
35-43: Addomitemptyifinstance_typeis optional for scaling groups.If existing scaling groups can omit this field, serialize it as optional to avoid emitting empty strings and to stay consistent with other domain structs.
✅ Suggested change
- InstanceType string `json:"instance_type"` // NEW: Configuration type + InstanceType string `json:"instance_type,omitempty"` // NEW: Configuration typeAs per coding guidelines, include
jsontags withomitemptyfor optional fields.internal/core/domain/container.go (1)
24-32: Addomitemptyifinstance_typeis optional for deployments.If the field is not mandatory, omit it in JSON to avoid emitting empty strings and align with the guideline for optional fields.
✅ Suggested change
- InstanceType string `json:"instance_type"` // NEW: Configuration type + InstanceType string `json:"instance_type,omitempty"` // NEW: Configuration typeAs per coding guidelines, include
jsontags withomitemptyfor optional fields.internal/core/ports/compute_options.go (1)
13-15: Add JSON tags for new limit fields (if these options are serialized).This keeps zero values out of payloads and aligns with the struct-tag convention for optional fields.
As per coding guidelines Include `json` tags on struct fields with `omitempty` for optional fields.🔧 Proposed update
- CPULimit int64 // CPU cores (or millicores) - MemoryLimit int64 // Memory in bytes - DiskLimit int64 // Disk in bytes + CPULimit int64 `json:"cpu_limit,omitempty"` // CPU cores (or millicores) + MemoryLimit int64 `json:"memory_limit,omitempty"` // Memory in bytes + DiskLimit int64 `json:"disk_limit,omitempty"` // Disk in bytesdocs/implementation.md (1)
62-64: Clarify name-based lookup in the API section.The interface now says
idOrName, while the later API examples still show/instances/{id}. Consider adding a short note that names are accepted to avoid confusion.docs/swagger/swagger.json (1)
4948-4950: Consider documenting validinstance_typevalues.A short description or enum (e.g., basic/standard/performance variants) will improve API discoverability for clients.
Also applies to: 5494-5497, 6349-6351
internal/core/services/stack.go (1)
276-297: Stack-created instances lack instance type and use hardcoded port.Two concerns with the
createInstancecall:
Empty
instanceType: Passing""means stack-created instances won't have resource limits (CPU, Memory, Disk) applied. Consider reading anInstanceTypeproperty from the template or defaulting to a sensible value like"basic-2".Hardcoded port
"80": The port should be read fromprops["Ports"]similar to howName,Image, andVpcIDare extracted.Suggested improvement
func (s *stackService) createInstance(ctx context.Context, stackID uuid.UUID, logicalID string, props map[string]interface{}) (uuid.UUID, error) { name, _ := props["Name"].(string) image, _ := props["Image"].(string) + ports, _ := props["Ports"].(string) + instanceType, _ := props["InstanceType"].(string) vpcID, _ := props["VpcID"].(uuid.UUID) if name == "" { name = fmt.Sprintf("%s-%s", logicalID, stackID.String()[:8]) } + if ports == "" { + ports = "80" + } + if instanceType == "" { + instanceType = "basic-2" + } var vpcIDPtr *uuid.UUID if vpcID != uuid.Nil { vpcIDPtr = &vpcID } - inst, err := s.instanceSvc.LaunchInstance(ctx, name, image, "80", "", vpcIDPtr, nil, nil) + inst, err := s.instanceSvc.LaunchInstance(ctx, name, image, ports, instanceType, vpcIDPtr, nil, nil)pkg/sdk/compute_test.go (1)
73-101: Consider verifyinginstance_typein the request body.The test correctly passes
"basic-2"as the newinstanceTypeargument, but the request body assertions (lines 86-87) only checknameandimage. For completeness, consider also asserting thatinstance_typeis included in the payload.Optional enhancement
assert.Equal(t, computeNewInstance, body["name"]) assert.Equal(t, "nginx", body["image"]) + assert.Equal(t, "basic-2", body["instance_type"])docs/swagger/swagger.yaml (1)
727-729: Consider removing the "NEW:" prefix from the description.The description "NEW: Configuration type" is informal for API documentation. Once merged, users won't benefit from knowing it's "new." A cleaner description would be simply "Instance type for scaling group members" or "Configuration type for instances."
cmd/cloud/instance.go (1)
172-182: Consider displaying instance type in theshowcommand output.The
showcommand displays various instance details but omits the instance type. For consistency with the new feature, consider adding it:Suggested addition
fmt.Printf(fmtDetailRow, "Image:", inst.Image) +fmt.Printf(fmtDetailRow, "Type:", inst.InstanceType) // After SDK Instance struct is updated fmt.Printf(fmtDetailRow, "Ports:", inst.Ports)This depends on the SDK's
Instancestruct being updated to includeInstanceType.internal/repositories/postgres/instance_repo.go (1)
29-35: Consider normalizing emptyinstance_typeto NULL on writes.If
instance_typeis optional, storing empty strings can make “unset” values ambiguous. UsingNULLIFkeeps behavior consistent with yourCOALESCEreads and optional-field semantics.♻️ Suggested change
- VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, NULLIF($11, '')::inet, $12, $13, $14, $15, $16) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, NULLIF($11, '')::inet, $12, NULLIF($13, ''), $14, $15, $16) ... - SET name = $1, status = $2, version = version + 1, updated_at = $3, container_id = $4, ports = $5, vpc_id = $6, subnet_id = $7, private_ip = NULLIF($8, '')::inet, ovs_port = $9, instance_type = $10 + SET name = $1, status = $2, version = version + 1, updated_at = $3, container_id = $4, ports = $5, vpc_id = $6, subnet_id = $7, private_ip = NULLIF($8, '')::inet, ovs_port = $9, instance_type = NULLIF($10, '')Also applies to: 114-118
internal/repositories/postgres/migrations/075_add_instance_type.up.sql (1)
1-26: Consider adding a foreign key constraint for data integrity.The
instances.instance_typecolumn referencesinstance_types.id, but there's no foreign key constraint enforcing this relationship. This could allow invalid instance types to be stored.🔧 Proposed fix to add foreign key constraint
ALTER TABLE instances ADD COLUMN IF NOT EXISTS instance_type TEXT DEFAULT 'basic-2'; + +-- Add foreign key after seeding instance_types to ensure referential integrity +-- Note: Add this after the INSERT statements or use DEFERRABLE constraintAfter the seed INSERT statements, add:
ALTER TABLE instances ADD CONSTRAINT fk_instances_instance_type FOREIGN KEY (instance_type) REFERENCES instance_types(id);If the FK constraint is intentionally omitted (e.g., to allow custom/dynamic instance types not in the reference table), please confirm and document this design decision.
internal/core/services/instance_test.go (2)
20-24: Lift instance-type resource literals into named constants.The VCPU/Memory numbers are repeated magic values; naming them improves intent and keeps the test data consistent.
As per coding guidelines: Do not use magic numbers - use named constants instead.♻️ Proposed constants
const ( testPorts = "8080:80" testVPCNetwork = "cloud-network" defaultInstanceType = "basic-2" + defaultInstanceVCPUs = 2 + defaultInstanceMemoryMB = 4096 ) -itRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.InstanceType{ID: defaultInstanceType, VCPUs: 2, MemoryMB: 4096}, nil).Maybe() +itRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.InstanceType{ID: defaultInstanceType, VCPUs: defaultInstanceVCPUs, MemoryMB: defaultInstanceMemoryMB}, nil).Maybe() -itRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.InstanceType{ID: defaultInstanceType, VCPUs: 2, MemoryMB: 4096}, nil).Maybe() +itRepo.On("GetByID", mock.Anything, mock.Anything).Return(&domain.InstanceType{ID: defaultInstanceType, VCPUs: defaultInstanceVCPUs, MemoryMB: defaultInstanceMemoryMB}, nil).Maybe()Also applies to: 101-101, 417-417
265-304: Convert Exec tests to a table-driven test.These cases are structurally identical and fit the table-driven style required for tests.
As per coding guidelines: Use table-driven tests in test files.♻️ Example table-driven rewrite
-func TestExecInstanceNotRunning(t *testing.T) { - repo, _, _, _, _, _, _, _, _, _, svc := setupInstanceServiceTest(t) - defer repo.AssertExpectations(t) - - instID := uuid.New() - inst := &domain.Instance{ID: instID, ContainerID: ""} - repo.On("GetByID", mock.Anything, instID).Return(inst, nil) - - _, err := svc.Exec(context.Background(), instID.String(), []string{"echo", "hi"}) - assert.Error(t, err) -} - -func TestExecInstanceSuccess(t *testing.T) { - repo, _, _, _, compute, _, _, _, _, _, svc := setupInstanceServiceTest(t) - defer repo.AssertExpectations(t) - defer compute.AssertExpectations(t) - - instID := uuid.New() - inst := &domain.Instance{ID: instID, ContainerID: "c123"} - repo.On("GetByID", mock.Anything, instID).Return(inst, nil) - compute.On("Exec", mock.Anything, "c123", []string{"echo", "hi"}).Return("hi", nil) - - out, err := svc.Exec(context.Background(), instID.String(), []string{"echo", "hi"}) - assert.NoError(t, err) - assert.Equal(t, "hi", out) -} - -func TestExecInstanceError(t *testing.T) { - repo, _, _, _, compute, _, _, _, _, _, svc := setupInstanceServiceTest(t) - defer repo.AssertExpectations(t) - defer compute.AssertExpectations(t) - - instID := uuid.New() - inst := &domain.Instance{ID: instID, ContainerID: "c123"} - repo.On("GetByID", mock.Anything, instID).Return(inst, nil) - compute.On("Exec", mock.Anything, "c123", []string{"echo", "hi"}).Return("", assert.AnError) - - _, err := svc.Exec(context.Background(), instID.String(), []string{"echo", "hi"}) - assert.Error(t, err) -} +func TestExecInstance(t *testing.T) { + cases := []struct { + name string + containerID string + execOut string + execErr error + wantErr bool + }{ + {name: "NotRunning", containerID: "", wantErr: true}, + {name: "Success", containerID: "c123", execOut: "hi", wantErr: false}, + {name: "ComputeError", containerID: "c123", execErr: assert.AnError, wantErr: true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + repo, _, _, _, compute, _, _, _, _, _, svc := setupInstanceServiceTest(t) + defer repo.AssertExpectations(t) + if tc.containerID != "" { + defer compute.AssertExpectations(t) + } + + instID := uuid.New() + inst := &domain.Instance{ID: instID, ContainerID: tc.containerID} + repo.On("GetByID", mock.Anything, instID).Return(inst, nil) + if tc.containerID != "" { + compute.On("Exec", mock.Anything, tc.containerID, []string{"echo", "hi"}).Return(tc.execOut, tc.execErr) + } + + out, err := svc.Exec(context.Background(), instID.String(), []string{"echo", "hi"}) + if tc.wantErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.execOut, out) + }) + } +}
| ### GET /instance-types | ||
| List all available instance types. | ||
| **Response:** | ||
| ```json | ||
| [ | ||
| { | ||
| "id": "basic-2", | ||
| "name": "Basic 2", | ||
| "vcpus": 1, | ||
| "memory_mb": 1024, | ||
| "disk_gb": 10 | ||
| } | ||
| ] | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "InstanceType" --type go -A 15 internal/core/domain/instance_type.goRepository: PoyrazK/thecloud
Length of output: 680
🏁 Script executed:
rg -n "instance-types|/instance-types" --type go -B 3 -A 15 | head -80Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
rg -n "GetInstanceTypes|instance.*types|InstanceType" --type go | grep -i "handler\|func\|route" | head -30Repository: PoyrazK/thecloud
Length of output: 1798
🏁 Script executed:
rg -n "instance.*type|GET.*instance" --type go -B 2 -A 10 | grep -A 10 -B 2 "handler\|func.*Get\|router\|Route" | head -60Repository: PoyrazK/thecloud
Length of output: 4465
🏁 Script executed:
fd -name "*.go" internal/ | xargs rg -l "instance-type\|InstanceType.*http\|GET" | head -10Repository: PoyrazK/thecloud
Length of output: 290
🏁 Script executed:
fd "instance.*type.*handler" --type f internal/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
rg "InstanceType.*\(\)" --type go -B 5 -A 15 internal/handlers/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
fd "main.go|routes.go|server.go" --type f internal/ cmd/Repository: PoyrazK/thecloud
Length of output: 139
🏁 Script executed:
rg "instance-types|GetInstanceTypes" --type go -B 3 -A 10 internal/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
head -100 cmd/api/main.goRepository: PoyrazK/thecloud
Length of output: 3843
🏁 Script executed:
rg -n "SetupRoutes\|router\|Route\|instance" --type go -A 5 cmd/api/main.go | head -50Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
fd "setup\|router" --type f internal/api/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
cat internal/api/setup.go 2>/dev/null || cat internal/api/setup/setup.go 2>/dev/null || ls -la internal/api/Repository: PoyrazK/thecloud
Length of output: 805
🏁 Script executed:
ls -la internal/api/setup/Repository: PoyrazK/thecloud
Length of output: 628
🏁 Script executed:
rg "SetupRouter\|instance-types\|InstanceType" --type go internal/api/setup/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
rg -n "instance-types\|instance_type\|InstanceType" --type go -B 2 -A 5 internal/api/setup/router.goRepository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
cat internal/api/setup/router.go | head -150Repository: PoyrazK/thecloud
Length of output: 5987
🏁 Script executed:
grep -n "instance-types\|GET.*instance\|instance.*routes" internal/api/setup/router.go -A 5 -B 2Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
cat internal/api/setup/router.go | tail -n +150 | head -200Repository: PoyrazK/thecloud
Length of output: 11017
🏁 Script executed:
grep -n "instance-types" internal/api/setup/router.goRepository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
rg "registerAdminRoutes\|registerDevOpsRoutes" --type go -A 100 internal/api/setup/router.go | head -150Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
wc -l internal/api/setup/router.goRepository: PoyrazK/thecloud
Length of output: 92
🏁 Script executed:
tail -300 internal/api/setup/router.goRepository: PoyrazK/thecloud
Length of output: 18842
🏁 Script executed:
sed -n '170,195p' docs/api-reference.mdRepository: PoyrazK/thecloud
Length of output: 456
🏁 Script executed:
rg "/instance-types|ListInstanceTypes|GetInstanceTypes" --type go -B 3 -A 10 internal/Repository: PoyrazK/thecloud
Length of output: 42
🏁 Script executed:
rg "InstanceType.*List\|List.*InstanceType" --type go -B 2 -A 10 internal/handlers/Repository: PoyrazK/thecloud
Length of output: 42
Document all InstanceType fields in the API response schema.
The InstanceType domain model includes network_mbps, price_per_hour, and category fields with JSON tags, so they will be serialized in API responses. Add these fields to the documented response schema for completeness.
🤖 Prompt for AI Agents
In `@docs/api-reference.md` around lines 176 - 189, The documented response for
GET /instance-types is missing fields present on the InstanceType model; update
the example JSON and schema to include network_mbps, price_per_hour, and
category so it matches the serialized API output. Locate the GET /instance-types
section and the InstanceType examples and add the network_mbps (number),
price_per_hour (number/string as used in serialization), and category (string)
properties to each instance object and any schema tables or descriptions
referencing InstanceType.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/core/services/instance_type.go`:
- Around line 10-21: Add unit tests for the instance type service by creating
internal/core/services/instance_type_test.go that exercises
NewInstanceTypeService and its List method: implement a mock of
ports.InstanceTypeRepository that you can configure to return a slice of
*domain.InstanceType and nil error (and a case returning an error), construct
the service with NewInstanceTypeService(mockRepo), call List(ctx) and assert the
returned results and errors match expectations; include tests for successful
listing and repository error propagation, and use context.Background() for calls
and table-driven style to keep tests concise.
In `@internal/handlers/instance_type_handler_test.go`:
- Line 54: The json.Unmarshal call currently discards its error; change it to
capture and assert the error instead of using the blank identifier—e.g., replace
the call to json.Unmarshal(data, &actualTypes) with err := json.Unmarshal(data,
&actualTypes) and then assert/check the error (use require.NoError(t, err) or
t.Fatalf/ t.Fatal if err != nil) so failures surface in the test; reference the
json.Unmarshal call and the actualTypes variable when making the change.
🧹 Nitpick comments (1)
internal/handlers/instance_type_handler_test.go (1)
29-59: Consider adding error case test and using table-driven structure.The success case is well-tested. Consider adding:
- An error case when the service returns an error
- Table-driven test structure for better coverage
♻️ Suggested table-driven approach with error case
func TestInstanceTypeHandlerList(t *testing.T) { gin.SetMode(gin.TestMode) - mockSvc := new(instanceTypeServiceMock) - handler := NewInstanceTypeHandler(mockSvc) - r := gin.New() - r.GET("/instance-types", handler.List) - t.Run("success", func(t *testing.T) { - expectedTypes := []*domain.InstanceType{ - {ID: "basic-1", Name: "Basic Small", VCPUs: 1, MemoryMB: 1024, DiskGB: 10}, - } - mockSvc.On("List", mock.Anything).Return(expectedTypes, nil).Once() + tests := []struct { + name string + mockReturn []*domain.InstanceType + mockErr error + expectedStatus int + }{ + { + name: "success", + mockReturn: []*domain.InstanceType{{ID: "basic-1", Name: "Basic Small", VCPUs: 1, MemoryMB: 1024, DiskGB: 10}}, + mockErr: nil, + expectedStatus: http.StatusOK, + }, + { + name: "service error", + mockReturn: nil, + mockErr: errors.New(errors.Internal, "database error"), + expectedStatus: http.StatusInternalServerError, + }, + } - req, _ := http.NewRequest(http.MethodGet, "/instance-types", nil) - w := httptest.NewRecorder() - r.ServeHTTP(w, req) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockSvc := new(instanceTypeServiceMock) + handler := NewInstanceTypeHandler(mockSvc) + r := gin.New() + r.GET("/instance-types", handler.List) - assert.Equal(t, http.StatusOK, w.Code) - // ... rest of assertions - mockSvc.AssertExpectations(t) - }) + mockSvc.On("List", mock.Anything).Return(tt.mockReturn, tt.mockErr).Once() + + req, _ := http.NewRequest(http.MethodGet, "/instance-types", nil) + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + + assert.Equal(t, tt.expectedStatus, w.Code) + mockSvc.AssertExpectations(t) + }) + } }
| type instanceTypeService struct { | ||
| repo ports.InstanceTypeRepository | ||
| } | ||
|
|
||
| // NewInstanceTypeService creates a new InstanceTypeService. | ||
| func NewInstanceTypeService(repo ports.InstanceTypeRepository) ports.InstanceTypeService { | ||
| return &instanceTypeService{repo: repo} | ||
| } | ||
|
|
||
| func (s *instanceTypeService) List(ctx context.Context) ([]*domain.InstanceType, error) { | ||
| return s.repo.List(ctx) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for test files related to instance_type service
fd -t f 'instance_type.*test\.go' internal/core/services/
fd -t f '.*_test\.go' internal/core/services/ | xargs grep -l 'instanceTypeService\|NewInstanceTypeService\|InstanceTypeService' 2>/dev/null || echo "No matches found"Repository: PoyrazK/thecloud
Length of output: 76
Service implementation is structurally sound, but tests are required.
The code correctly:
- Uses an unexported struct with exported constructor
- Imports only
portsanddomainpackages - Delegates to repository via the port interface
- Passes context properly
However, per coding guidelines, new services must include tests. Create internal/core/services/instance_type_test.go with test coverage for List() method using mocked repositories.
🤖 Prompt for AI Agents
In `@internal/core/services/instance_type.go` around lines 10 - 21, Add unit tests
for the instance type service by creating
internal/core/services/instance_type_test.go that exercises
NewInstanceTypeService and its List method: implement a mock of
ports.InstanceTypeRepository that you can configure to return a slice of
*domain.InstanceType and nil error (and a case returning an error), construct
the service with NewInstanceTypeService(mockRepo), call List(ctx) and assert the
returned results and errors match expectations; include tests for successful
listing and repository error propagation, and use context.Background() for calls
and table-driven style to keep tests concise.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/core/services/instance_test.go (1)
528-537:⚠️ Potential issue | 🟡 MinorRemove incomplete test stub or implement it properly with assertions.
The test sets up a mock but never invokes any code or asserts results, so it always passes without validation. Since
getVolumeByIDOrNameis unexported and only called byresolveVolumes(which is called byLaunchInstance), test volume resolution throughLaunchInstanceinstead—similar to the existingTestInstanceServiceLaunchWithVolumestest, which properly invokes the service and asserts behavior using table-driven test cases per coding guidelines. Either implement this test with actual invocations and assertions or remove it.
🤖 Fix all issues with AI agents
In `@internal/core/services/instance.go`:
- Around line 98-105: The instance-type lookup currently maps all errors from
s.instanceTypeRepo.GetByID to errors.InvalidInput; change this to distinguish
not-found from internal failures: after calling s.instanceTypeRepo.GetByID(ctx,
instanceType) check whether the returned error represents a not-found (e.g.,
errors.Is(err, repo.ErrNotFound) or errors.Is(err, sql.ErrNoRows) or a
repository-provided IsNotFound helper) and only then return errors.InvalidInput
with the "invalid instance type" message; for any other non-nil error return an
internal/server error (e.g., errors.Internal) preserving or wrapping the
original error for logging/diagnostics. Ensure you reference the same symbols
s.instanceTypeRepo.GetByID, instanceType, and the error constructors
(errors.InvalidInput / errors.Internal) when making this change.
🧹 Nitpick comments (4)
pkg/sdk/compute.go (1)
51-60: Optionally omitinstance_typewhen empty.
This avoids sending an empty string and lets the server default cleanly.♻️ Suggested adjustment
body := map[string]interface{}{ "name": name, "image": image, "ports": ports, - "instance_type": instanceType, "vpc_id": vpcID, "subnet_id": subnetID, "volumes": volumes, } +if instanceType != "" { + body["instance_type"] = instanceType +}internal/repositories/noop/adapters.go (1)
42-51: Replace magic numbers with named constants for noop instance types.
This keeps test fixtures self-describing and consistent.As per coding guidelines: Do not use magic numbers - use named constants instead.♻️ Suggested adjustment
func (r *NoopInstanceTypeRepository) List(ctx context.Context) ([]*domain.InstanceType, error) { + const ( + basic1VCPUs = 1 + basic1MemoryMB = 512 + basic1DiskGB = 8 + basic2VCPUs = 1 + basic2MemoryMB = 1024 + basic2DiskGB = 10 + ) return []*domain.InstanceType{ - {ID: "basic-1", Name: "Basic 1", VCPUs: 1, MemoryMB: 512, DiskGB: 8}, - {ID: "basic-2", Name: "Basic 2", VCPUs: 1, MemoryMB: 1024, DiskGB: 10}, + {ID: "basic-1", Name: "Basic 1", VCPUs: basic1VCPUs, MemoryMB: basic1MemoryMB, DiskGB: basic1DiskGB}, + {ID: "basic-2", Name: "Basic 2", VCPUs: basic2VCPUs, MemoryMB: basic2MemoryMB, DiskGB: basic2DiskGB}, }, nil }internal/core/services/instance.go (1)
175-177: Use named constants for byte multipliers.
This improves readability and aligns with the no-magic-numbers guideline.As per coding guidelines: Do not use magic numbers - use named constants instead.♻️ Suggested adjustment
- cpuLimit := int64(it.VCPUs) - memLimit := int64(it.MemoryMB) * 1024 * 1024 - diskLimit := int64(it.DiskGB) * 1024 * 1024 * 1024 + const ( + bytesPerMB = 1024 * 1024 + bytesPerGB = 1024 * 1024 * 1024 + ) + cpuLimit := int64(it.VCPUs) + memLimit := int64(it.MemoryMB) * bytesPerMB + diskLimit := int64(it.DiskGB) * bytesPerGBinternal/handlers/instance_handler.go (1)
41-47: Addomitemptyto optional request fields.
Ports,VpcID,SubnetID, andVolumesare optional (nobinding:"required"), so their JSON tags should includeomitemptyto match the project convention. As per coding guidelines: Includejsontags on struct fields withomitemptyfor optional fields.♻️ Suggested change
- Ports string `json:"ports"` - VpcID string `json:"vpc_id"` - SubnetID string `json:"subnet_id"` - Volumes []VolumeAttachmentRequest `json:"volumes"` + Ports string `json:"ports,omitempty"` + VpcID string `json:"vpc_id,omitempty"` + SubnetID string `json:"subnet_id,omitempty"` + Volumes []VolumeAttachmentRequest `json:"volumes,omitempty"`
| // 2. Resolve Instance Type | ||
| if instanceType == "" { | ||
| instanceType = "basic-2" | ||
| } | ||
| _, err = s.instanceTypeRepo.GetByID(ctx, instanceType) | ||
| if err != nil { | ||
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid instance type: %s", instanceType)) | ||
| } |
There was a problem hiding this comment.
Don’t map all instance-type lookup failures to InvalidInput.
If the repo fails (e.g., DB outage), callers will see a 400 instead of an internal error and may not retry. Distinguish NotFound vs internal failures.
🛠️ Suggested handling
-import (
+import (
+ stderrors "errors"
"context"
"encoding/json"
"fmt"
"io" _, err = s.instanceTypeRepo.GetByID(ctx, instanceType)
if err != nil {
- return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid instance type: %s", instanceType))
+ var derr errors.Error
+ if stderrors.As(err, &derr) && derr.Type == errors.NotFound {
+ return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid instance type: %s", instanceType))
+ }
+ return nil, errors.Wrap(errors.Internal, "failed to resolve instance type", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 2. Resolve Instance Type | |
| if instanceType == "" { | |
| instanceType = "basic-2" | |
| } | |
| _, err = s.instanceTypeRepo.GetByID(ctx, instanceType) | |
| if err != nil { | |
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid instance type: %s", instanceType)) | |
| } | |
| // 2. Resolve Instance Type | |
| if instanceType == "" { | |
| instanceType = "basic-2" | |
| } | |
| _, err = s.instanceTypeRepo.GetByID(ctx, instanceType) | |
| if err != nil { | |
| var derr errors.Error | |
| if stderrors.As(err, &derr) && derr.Type == errors.NotFound { | |
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("invalid instance type: %s", instanceType)) | |
| } | |
| return nil, errors.Wrap(errors.Internal, "failed to resolve instance type", err) | |
| } |
🤖 Prompt for AI Agents
In `@internal/core/services/instance.go` around lines 98 - 105, The instance-type
lookup currently maps all errors from s.instanceTypeRepo.GetByID to
errors.InvalidInput; change this to distinguish not-found from internal
failures: after calling s.instanceTypeRepo.GetByID(ctx, instanceType) check
whether the returned error represents a not-found (e.g., errors.Is(err,
repo.ErrNotFound) or errors.Is(err, sql.ErrNoRows) or a repository-provided
IsNotFound helper) and only then return errors.InvalidInput with the "invalid
instance type" message; for any other non-nil error return an internal/server
error (e.g., errors.Internal) preserving or wrapping the original error for
logging/diagnostics. Ensure you reference the same symbols
s.instanceTypeRepo.GetByID, instanceType, and the error constructors
(errors.InvalidInput / errors.Internal) when making this change.
1 participant
This PR introduces support for instance types and resource limits across the platform.
Key Changes:
All tests passed successfully.
Summary by CodeRabbit
Release Notes
New Features
--type(-t) CLI flag to the instance launch command (default: basic-2).GET /instance-typesAPI endpoint to list available instance types with resource specifications and pricing.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.