-
Notifications
You must be signed in to change notification settings - Fork 1
Add service drivers to Magic #31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Services are now actually managed by the runner - Renamed postgres driver to postgres_legacy (and separated it into more files) - EnvironmentValue support for the Postgres Driver - Implemented the new instruction layer into the Runner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors Magic’s container management from a database-specific model to a generic service driver model, updating plan generation/deployment and introducing a legacy PostgreSQL service driver implementation.
Changes:
- Introduces
mconfig.ServiceDriver+ registry and migrates planning toPlan.Containerskeyed by driver unique IDs. - Reworks deployment to pull/start/health-check/initialize service containers (concurrently) and adds runtime instructions (clear/drop tables).
- Removes the old
integration/*helpers and the legacymconfig/databases.godatabase abstraction; updates example project usage accordingly.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| util/util.go | Adds random port selection + port scan helper used during planning. |
| mrunner/runner_plan.go | Replaces DB planning with service-driver container + port allocation planning. |
| mrunner/runner_deploy.go | Replaces DB deployment with generic service container pull/start/health/initialize + instruction dispatch. |
| mrunner/runner.go | Runner now tracks service drivers and updates Docker client initialization. |
| mrunner/databases/postgres_legacy.go | Adds legacy Postgres driver (v14–17) and env helpers for host/port/user/pass. |
| mrunner/databases/postgres_legacy_container.go | Implements container create/health/initialize for legacy Postgres driver. |
| mrunner/databases/postgres_legacy_instruct.go | Implements “clear tables” / “drop tables” instructions for legacy Postgres. |
| mrunner/databases/init.go | Registers the Postgres driver in the global driver registry. |
| mconfig/services.go | Adds ServiceDriver interface, instruction types, container allocation/info structs, and registry helpers. |
| mconfig/plan.go | Replaces DB-type plan with Containers map + generic container naming helpers. |
| mconfig/environment.go | Adds ValueFunction helper for dynamic environment values. |
| mconfig/databases.go | Removes legacy database abstraction layer. |
| mconfig/context.go | Context now tracks services (drivers) and project directory; removes DB APIs. |
| integration/sanitize_path.go | Removed legacy integration helper. |
| integration/port_scanner.go | Removed legacy integration helper (port scanning moved to util). |
| integration/path_evaluator_test.go | Removed legacy integration test file. |
| integration/path_evaluator.go | Removed legacy integration helper. |
| integration/formatting.go | Removed legacy integration helper. |
| integration/file.go | Removed legacy integration helper. |
| integration/execute_command.go | Removed legacy integration helper. |
| integration/directory.go | Removed legacy integration helper. |
| integration/constants.go | Removed legacy integration constant. |
| initializer.go | Updates context creation to include project directory. |
| factory.go | Updates plan filename format and adjusts project dir discovery loop. |
| examples/real-project/starter/start_test.go | Updates test helper to use ClearTables() API. |
| examples/real-project/starter/scripts_database.go | Adds clear vs reset scripts using new instruction APIs. |
| examples/real-project/starter/config.go | Updates example to register Postgres service driver and use driver env helpers. |
Comments suppressed due to low confidence (2)
mrunner/runner_deploy.go:259
- If
ContainerInspectfails, the code still accessescontainerInfo.Container.Mounts. Whenerr != nil,containerInfo.Containercan be nil, causing a panic. Guard mount/volume extraction behindif err == nil && containerInfo.Container != nil, or return the inspect error before using the response.
// Get all the attached volumes to delete them manually
containerInfo, err := r.client.ContainerInspect(ctx, containerID, client.ContainerInspectOptions{})
if err != nil {
util.Log.Println("Warning: Couldn't inspect container:", err)
}
var volumeNames []string
if containerInfo.Container.Mounts != nil {
for _, mnt := range containerInfo.Container.Mounts {
if mnt.Type == mount.TypeVolume && mnt.Name != "" {
volumeNames = append(volumeNames, mnt.Name)
}
mrunner/runner_plan.go:67
- In the host port allocation loop,
slices.Contains(portsToAllocate, port)is always true for the currentport(since you're iterating overportsToAllocate). That means the condition effectively only checks!util.ScanPort(toAllocate)and never ensurestoAllocateisn’t already reserved elsewhere inportsToAllocate. This can yield duplicate allocations. Useslices.Contains(portsToAllocate, toAllocate)(or a set) and treatScanPort+ uniqueness as separate constraints.
if len(portsToAllocate) >= 0 {
for _, port := range portsToAllocate {
// Generate a new port in case the current one is taken
toAllocate := port
for slices.Contains(portsToAllocate, port) && !util.ScanPort(toAllocate) {
toAllocate = util.RandomPort(DefaultStartPort, DefaultEndPort)
}
// Add the port to the plan
allocatedPorts[port] = toAllocate
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
No description provided.