Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 6 commits into from
Oct 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0999ec9
fix(serve): connect to Cloud if process is started outside of project…
edvald Jul 13, 2023
9b45b6d
chore: fix build
shumailxyz Oct 4, 2023
379d53a
fix: set localServerPort for only non dev/serve commands
shumailxyz Oct 4, 2023
694565c
chore: updating typings to enforce explicit value for localServerPort
shumailxyz Oct 5, 2023
ac9f167
Merge branch 'main' into connect-cloud-on-demand
shumailxyz Oct 5, 2023
e39bcab
Merge branch 'main' into connect-cloud-on-demand
shumailxyz Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/commands/serve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export class ServeCommand<
const session = await cloudApi.registerSession({
parentSessionId: undefined,
projectId,
// Use the process (i.e. parent command) session ID for the serve command session
// Use the process (i.e. parent command) session ID for the serve/dev command session
sessionId: manager.sessionId,
commandInfo: garden.commandInfo,
localServerPort: this.server.port,
Expand Down
25 changes: 23 additions & 2 deletions core/src/server/instance-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ interface GardenInstanceManagerParams {
log: Log
sessionId: string
plugins: GardenPluginReference[]
serveCommand?: ServeCommand
serveCommand: ServeCommand
extraCommands?: Command[]
defaultOpts?: Partial<GardenOpts>
cloudApiFactory?: CloudApiFactory
Expand Down Expand Up @@ -104,6 +104,7 @@ export class GardenInstanceManager {
this.defaultOpts = defaultOpts || {}
this.plugins = plugins
this.cloudApiFactory = cloudApiFactory || CloudApi.factory
this.serveCommand = serveCommand

this.events = new EventBus()
this.monitors = new MonitorManager(log, this.events)
Expand Down Expand Up @@ -381,7 +382,7 @@ export class GardenInstanceManager {

const gardenParams = await resolveGardenParamsPartial(projectRoot, gardenOpts)

return this.ensureInstance(
const garden = await this.ensureInstance(
log,
{
projectRoot,
Expand All @@ -391,5 +392,25 @@ export class GardenInstanceManager {
},
gardenOpts
)

if (cloudApi && garden.projectId && this.serveCommand?.server) {
// Ensure cloud session is registered for the domain and server session, since this may not happen on startup
// if the command isn't started in a Garden project root. This is a no-op if it's already registered.
// FIXME: We still need to rethink on the Cloud side how sessions are scoped
await cloudApi.registerSession({
parentSessionId: undefined,
projectId: garden.projectId,
// Use the process (i.e. parent command) session ID for the serve/dev command session
sessionId: this.sessionId,
commandInfo: garden.commandInfo,
// set localServerPort only for dev/serve commands
localServerPort: ["dev", "serve"].includes(garden.commandInfo.name) ? this.serveCommand.server.port : undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@shumailxyz shumailxyz Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha ok. Makes sense.

environment: garden.environmentName,
namespace: garden.namespace,
isDevCommand: true,
})
}

return garden
}
}