fix(serve): connect to Cloud if process is started outside of project dir

[edvald]

This came up in the Desktop client because it'd start the serve command outside of any project root, and thus not connect to Cloud at all for the live view etc. to work.

cc @eysi09

[vvagaytsev]

@eysi09 is this ready to do? LGTM, but I might miss some context.

[eysi09]

@eysi09 is this ready to do? LGTM, but I might miss some context.

I'm not sure. Maybe I'm missing something, but when we connect to Cloud we need to do that in the context of some specific project. What would that be in the case where Garden is started outside of a given project?

[trymbill]

@eysi09 and I just verified this works now for Desktop. So LGTM! 👍

See comment below

[eysi09]

@eysi09 and I just verified this works now for Desktop. So LGTM! 👍

There's actually one issue here with the localServerPort that requires a bit more thought.

The original intent was for it to only be set for the dev command (and 0.12 serve command) and it's used by cloud to determine whether the core server is running. If a port is set, it suggests the server is running, and cloud will try to connect.

With this change it looks like we're setting the port for non-server commands and that breaks that behaviour.

Maybe the fix is just to not set it for this particular code path but I'm not entirely sure if that makes sense or not. In any case it's not clear from the code as is why it should be omitted in this context.

I flagged the PR as a draft until we fix that.

[eysi09]

Just to follow up.

I think we just need to make sure we only set the localServerPort for the dev or serve command. So when registering a session for, say, a deploy command, we need to make sure that the local server port isn't set in the payload.

This hasn't been an issue until the change in this issue was introduced. Basically the added registerSession call appears to sometime set a server port when it shouldn't.

[eysi09]

This is called in more places and we need to update it all of them.

Can I also suggest that in the registerSession function we set the localServerPort type to number | null so that it's explicit and we won't forget to add it at other call sites.

[shumailxyz]

I think we don't need to update other places. We basically have 3 calls of registerSession including this one. The other 2 calls are in base.ts and serve.ts.

The base.ts call already has a check and skips the registration in case of dev/serve.

garden/core/src/commands/base.ts

Lines 311 to 315 in 8490aac

	const skipRegistration =
	!["dev", "serve"].includes(this.name) && this.maybePersistent(params) && !params.parentCommand
	if (!skipRegistration && garden.cloudApi && garden.projectId && this.streamEvents) {
	cloudSession = await garden.cloudApi.registerSession({

And in serve.ts, we do not need this check as it only registers session for dev/serve.

[eysi09]

Aha ok. Makes sense.