Skip to content

fix: use buildServerId for preview deployment build and service creation#3863

Open
kopandante wants to merge 2 commits intoDokploy:canaryfrom
kopandante:fix/preview-build-server-routing
Open

fix: use buildServerId for preview deployment build and service creation#3863
kopandante wants to merge 2 commits intoDokploy:canaryfrom
kopandante:fix/preview-build-server-routing

Conversation

@kopandante
Copy link

@kopandante kopandante commented Mar 2, 2026

Summary

Preview deployments ignore the buildServerId configuration, causing failures when a separate build server is configured. This PR fixes four bugs:

Bug 1 — buildRegistry unconditionally nulled (application.ts)
Both deployPreviewApplication and rebuildPreviewApplication set application.buildRegistry = null unconditionally. When a separate build server is configured, buildRegistry provides the registry credentials needed to push/pull images between servers. Nulling it breaks getAuthConfig() and getImageName() in mechanizeDockerContainer.

Fix: Only null buildRegistry when there is no separate build server (!buildServerId || buildServerId === serverId), matching the regular deploy flow where mechanizeDockerContainer(application) receives the original application with buildRegistry intact.

Bug 2 — Build runs on deploy server instead of build server (application.ts)
deployPreviewApplication uses application.serverId for execAsyncRemote, and rebuildPreviewApplication assigns const serverId = application.serverId. Both ignore buildServerId.

Fix: Use application.buildServerId || application.serverId, consistent with deployApplication (L178) and rebuildApplication (L296).

Bug 3 — createService doesn't pass auth (builders/index.ts)
docker.createService(settings) does not extract authconfig from the settings object. In contrast, service.update(opts) does extract opts.authconfig. This means newly created Swarm services cannot pull from authenticated registries.

Fix: docker.createService(settings.authconfig, settings) — pass auth as the first argument per the dockerode API signature.

Bug 4 — Log directory created on wrong server (deployment.ts)
createDeploymentPreview uses previewDeployment.application.serverId for paths(), findServerById(), and execAsyncRemote(). The log directory is created on the deploy server, but the build command runs on the build server, causing "file not found" errors.

Fix: Use buildServerId || serverId, consistent with createDeployment (L87).

Context

All four bugs only manifest when buildServerId is configured (separate build server). The regular deployment flow (deployApplication, rebuildApplication, createDeployment) already handles this correctly — preview deployments were simply never updated to use the same pattern.

These fixes are verified in production on our Dokploy instance (v0.27.1 with equivalent chunk patches) running 21 applications across two servers.

Supersedes #3784 (which mixed these fixes with unrelated changes).

Test plan

  • Configure an application with a separate build server and build registry
  • Create a PR to trigger preview deployment — verify build runs on build server
  • Verify logs are written to build server and visible in Dokploy UI
  • Verify the preview container starts successfully with registry auth
  • Rebuild the preview — verify same behavior
  • Deploy without build server configured — verify no regression

Greptile Summary

This PR correctly fixes preview deployments to respect buildServerId configuration, making them consistent with the regular deployment flow. The changes properly handle:

  • Using build server for build operations instead of deploy server
  • Preserving buildRegistry credentials when using separate build servers
  • Creating log directories on the correct server
  • Passing registry authentication to Docker service creation

However, the change to docker.createService(settings.authconfig, settings) in builders/index.ts:185 requires updating the test mocks and assertions in mechanizeDockerContainer.test.ts, as they currently expect a single-parameter call.

Confidence Score: 3/5

  • Safe to merge after fixing test suite compatibility
  • The code changes are logically sound and follow established patterns in the codebase. However, the tests will fail because the mock signature and assertions need updating to match the new two-parameter createService call. Once tests are fixed, this should work correctly.
  • Pay close attention to apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts - all test cases need updating to handle the new createService signature

Last reviewed commit: 40a68b0

(5/5) You can turn off certain types of comments like style here!

Preview deployments ignored the buildServerId configuration, causing
four issues when a separate build server was configured:

1. buildRegistry was unconditionally nulled, breaking registry auth
   for image push/pull between build and deploy servers

2. Build commands ran on serverId instead of buildServerId, so the
   build executed on the deploy server instead of the build server

3. docker.createService() did not pass authconfig as a separate
   argument (unlike service.update()), so Swarm could not pull
   images from authenticated registries

4. Log directory was created on the deploy server while the build
   ran on the build server, causing "file not found" errors

These fixes align preview deployment behavior with the regular
deployment flow (deployApplication/rebuildApplication) which already
correctly uses buildServerId.
@kopandante kopandante requested a review from Siumauricio as a code owner March 2, 2026 14:54
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 2, 2026
@dosubot dosubot bot added the bug Something isn't working label Mar 2, 2026
@dosubot
Copy link

dosubot bot commented Mar 2, 2026

Related Documentation

Checked 7 published document(s) in 1 knowledge base(s). No updates required.

How did I do? Any feedback?  Join Discord

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 2, 2026

Additional Comments (6)

apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
mock signature needs updating to match the new createService call in builders/index.ts:185 which now passes two parameters: authconfig and settings

		const createService = vi.fn<
			(authconfig: any, opts: MockCreateServiceOptions) => Promise<void>
		>(async () => undefined);

apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
createService now receives two arguments (authconfig, settings), need to destructure both

		const call = createServiceMock.mock.calls[0] as
			| [any, MockCreateServiceOptions]
			| undefined;
		if (!call) {
			throw new Error("createServiceMock should have been called once");
		}
		const [, settings] = call;

apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
same destructuring fix needed here

		const call = createServiceMock.mock.calls[0] as
			| [any, MockCreateServiceOptions]
			| undefined;
		if (!call) {
			throw new Error("createServiceMock should have been called once");
		}
		const [, settings] = call;

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
same here

		const call = createServiceMock.mock.calls[0];
		if (!call) {
			throw new Error("createServiceMock should have been called once");
		}
		const [, settings] = call;

apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
same here

		const call = createServiceMock.mock.calls[0];
		if (!call) {
			throw new Error("createServiceMock should have been called once");
		}
		const [, settings] = call;

apps/dokploy/__test__/server/mechanizeDockerContainer.test.ts
same here

		const call = createServiceMock.mock.calls[0];
		if (!call) {
			throw new Error("createServiceMock should have been called once");
		}
		const [, settings] = call;

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Mar 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant