-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat: move runner scaling to the provisioner #3529
Conversation
e106941
to
273f4d2
Compare
06b5022
to
0b0f22d
Compare
0aa9ae0
to
ba315f2
Compare
d36b8f2
to
ae351a3
Compare
@@ -149,6 +150,8 @@ func replaceOutputs(to []*provproto.Resource, from []*provproto.Resource) error | |||
} | |||
r.Subscription.Output = subscriptionFrom.Subscription.Output | |||
} | |||
case *provproto.Resource_Runner: | |||
// Ignore |
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.
Why ignoring here? Should we not just set the outpus?
backend/provisioner/registry.go
Outdated
@@ -273,9 +276,18 @@ func ExtractResources(msg *ftlv1.CreateDeploymentRequest) (*ResourceGraph, error | |||
to: dep.ResourceId, | |||
}) | |||
} | |||
|
|||
runnerResource := &provisioner.Resource{ | |||
ResourceId: module.GetName() + "-runner", |
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.
Should we use deployment id here instead? I think that would fit better to the given model, given that each deployment has a separate runner as a resource.
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.
Good point
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.
Actually we don't really seem to have the deployment name here?
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.
Hmm, yeah. I think it currently created after the provisioning, which is annoying. I suppose that is fine for now, though it breaks the model a bit.
074deb9
to
7eb038e
Compare
7eb038e
to
d54b736
Compare
fixes #3483