Run the Manager locally without Proxmox (make dev + mock provisioning)#329
Run the Manager locally without Proxmox (make dev + mock provisioning)#329horner wants to merge 5 commits into
Conversation
- Add make dev target: SQLite setup, deps, migrations, client build, serve - Create dev-user/dev-admin and localhost site on demand in dev login route - Remove localhost site seeder in favor of on-demand creation - Document Run the Manager Locally as single source of truth in dev workflow - Cross-reference from README and copilot-instructions - Ignore dev.sqlite and data/ local artifacts
Auto-create site, local node, and external domain on dev login so the new-container form is fully usable locally. Short-circuit Proxmox provisioning for the 'local' node (apiUrl === 'local') by mock-marking containers as running with placeholder VMID/MAC/IP. Fix SQLite-only erroneous UNIQUE constraint on Containers.nodeId that limited a node to one container; uniqueness is already enforced by the (nodeId, containerId) composite index. No-op on Postgres.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
runleveldev
left a comment
runleveldev
left a comment
There was a problem hiding this comment.
If it's not obvious per my other comments, I don't like this strategy. I understand the desire to rapidly iterate the web UI but with how heavily integrated this is to its hosting environment I don't believe any meaningful changes can be created without the ability to try them out against a real environment. The docker-compose workflow was intentionally designed to make doing so as painless as possible. In addition to my other comments, I believe supporting multiple development workflows will result in drift between team members and new contributors over time which will not happen only supporting the docker-compose workflow. Likewise, docker-compose ensures that development happens in a standard environment so the application is not accidentally made dependent on local development tools nor does it need code branches for alternate tool implementations or architectures (think awk flags on mac vs linux).
I reference these in my other comments, but as a summary, I will not approve this PR without the following changes:
- Postgres as the primary DB. Everyone has docker installed this should not be an issue
- Out-of-band signaling that a Node is mock preferably via a new field 'type' in the Node model set to 'dummy' (or similar) for a mock node
- An abstraction on the ProxmoxApi class allowing dummy nodes to be interacted with using the same code paths as Proxmox nodes
Even with those I still don't like the idea of mock APIs (how do you know your changes work against real APIs if you don't try them out?) but I will at least be okay with a unified implementation that doesn't require per-environment code paths.
There was a problem hiding this comment.
We are constantly fighting this bug with sqlite which is why production and the current docker compose stack use postgresql. I'm trying to avoid merging sqlite specific fixes with plans to remove sqlite support in a future PR. Officially Postgres is the only supported DB.
| const isAdmin = req.body?.role === 'admin'; | ||
| const uid = isAdmin ? 'dev-admin' : 'dev-user'; | ||
|
|
||
| // Ensure a localhost site exists to work with. |
There was a problem hiding this comment.
These new defaults look like they're going to conflict with the bootstrapping done by the Docker Compose setup. I'll need to be at my laptop to test it though.
There was a problem hiding this comment.
I confirmed this messed up the docker-compose bootstrap. It imports the nodes and containers into this localhost site instead of the intended "Development" site which has the IP ranges configured properly.
| // node (apiUrl === 'local') has no real hypervisor, so mark the container | ||
| // running immediately with placeholder VMID/MAC/IP. This is the hook point | ||
| // where a real Docker-based local provisioner can plug in later. | ||
| if (process.env.NODE_ENV !== 'production' && node.apiUrl === 'local') { |
There was a problem hiding this comment.
I don't think I can express enough how much of a bad idea I think this is. If changes are hitting this code path, they need to be QAed against a real Proxmox API. That's the whole point of the docker-compose setup. I have always taken a hard stance against mocks because its impossible to guarantee this is going to even remotely replicate what the actual code path is doing. Adding this will make my job as a code review significantly harder going forward because I will have no assurance that changes were ever tried against a Proxmox environment. I'm willing to entertain a lighterweight WebUI development workflow, but I won't approve mock providers since it drastically changes the assumptions about the environment.
There was a problem hiding this comment.
As a follow-up, I would be happier with this if there was an abstraction layer instead of a dedicated code path. Currently we just have the ProxmoxApi class, but I envision that providing an interface (NodeApi) which could then be implemented by a DummyApi class that the mock node uses. Code that requires interacting with Nodes would call through the appropriate Api class (determined by a new nodeType field). This would still require running the job-runner.js alongside the server if you need containers to get created, but that's a benefit to me since it ensures the full container creation code path if being exercised.
| .env | ||
| node_modules | ||
| data/ | ||
| *.sqlite |
There was a problem hiding this comment.
Why is this here if its already in the root .gitignore?
| }); | ||
|
|
||
| // Ensure a local dev node exists so containers can be created without | ||
| // a real Proxmox host. `apiUrl: 'local'` marks it as the mock node; |
There was a problem hiding this comment.
Just to reiterate my previous comment, I don't like mocks, but this implementation relying on in-band signaling is a massive anti-pattern since it overloads the use of that database field creating a convention contributors have to "just know" to understand why their mock is or isn't working. It's not really self-documenting. If I'm going to have to merge mock support, at least give me a dedicated field like 'dummyNode' (boolean) which only gets used if there's no real nodes available.
3 participants
Why
Until now, the only way to exercise the Manager (
create-a-container) web app was the full Docker stack (Proxmox, provisioning, DNS, reverse-proxy). That is slow to spin up and overkill for iterating on the web app itself. There was no way to:This PR makes the Manager fully usable on
localhostfor web-app development, with a clear hook point for a future Docker-based local provisioner.What changed
1.
make dev— run the Manager standalone (SQLite)devtarget sets up and startscreate-a-containeragainst local SQLite: creates.env(with a generatedSESSION_SECRET) if missing, installs deps, runs migrations, builds the client, and serves athttp://localhost:3000.db:migratescript to callsequelize-cli(thesequelizebin alias isn't installed).2. On-demand local environment on dev login
The
/devlogin route now self-provisions everything the new-container form needs, usingfindOrCreate(idempotent, dev-only):localhostSite,apiUrl: 'local') that acts as the mock hypervisor,localhost).The external domain is required because image templates that expose an HTTP port auto-add an HTTP service, and the form's validation requires a domain for HTTP services. Without it the new-container form was silently unsubmittable.
3. Mock provisioning for the local node
In
POST /containers, when running in dev against thelocalnode (apiUrl === 'local'), Proxmox provisioning is short-circuited: the container is immediately markedrunningwith a placeholder VMID/MAC/IP. This is the documented hook point where a real Docker-based local provisioner can plug in later.4. Bug fix — erroneous
UNIQUEonContainers.nodeIdSQLite's
changeColumnrebuilds the table and accidentally applied a column-levelUNIQUEonContainers.nodeId, which limited each node to a single container. A node (a Proxmox host) owns many containers; the correct uniqueness guarantee is already the(nodeId, containerId)composite index. New migration drops the bad constraint. No-op on Postgres, which was never affected (itsALTER COLUMNdoesn't rebuild tables).5. Docs
make dev)" section in the development workflow doc, with the Docker full-stack content demoted to a subsection.Testing
Verified end-to-end in the browser: dev login auto-provisions site/node/domain, then created multiple containers (Debian 13, NodeJS 24 with an HTTP service) — all land as
runningwith unique VMID/IP and no Proxmox required.