-
Notifications
You must be signed in to change notification settings - Fork 8
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
fix: redeploy all modules on schema changes #791
fix: redeploy all modules on schema changes #791
Conversation
cmd/ftl/cmd_dev.go
Outdated
@@ -38,6 +41,24 @@ func (d *devCmd) Run(ctx context.Context, client ftlv1connect.ControllerServiceC | |||
d.modules = make(map[string]moduleFolderInfo) | |||
d.client = client | |||
|
|||
stream, err := client.PullSchema(ctx, connect.NewRequest(&ftlv1.PullSchemaRequest{})) |
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.
This should be in the goroutine itself, and should handle disconnects of the stream. ie. there should be two loops, an outer one initiating the stream and an inner one pulling from the stream. Don't forget to close the stream if it errors.
Rebuilds are fast with Go, but not so much with Kotlin, so we should be smarter about this in future. Can you create a ticket to remind us to fix this? |
|
ae03344
to
a541198
Compare
cmd/ftl/cmd_dev.go
Outdated
continue | ||
} | ||
|
||
for { |
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.
A better pattern is:
for stream.Receive() {
msg := stream.Msg()
// ..
}
stream.Close()
// ...
a541198
to
003faf3
Compare
When the
ftl dev
command detects a schema change in any module, it will trigger a rebuild of all modules, including the one that caused the schema change.This isn't ideal because we might end up rebuilding a single module twice (if it caused a schema change), but the logic is simple and rebuilds are fast.