-
Notifications
You must be signed in to change notification settings - Fork 887
chore: allow terraform & echo built-in provisioners #13121
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
Conversation
I wonder if we can use the echo provisioner to provision local(on the client machine) workspaces. |
unlikely. The echo provisioner literally just echos a canned set of resources back to Coderd. It doesn't actually provision anything. |
cli/server.go
Outdated
@@ -944,15 +944,27 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd. | |||
var provisionerdWaitGroup sync.WaitGroup | |||
defer provisionerdWaitGroup.Wait() | |||
provisionerdMetrics := provisionerd.NewMetrics(options.PrometheusRegistry) | |||
for i := int64(0); i < vals.Provisioner.Daemons.Value(); i++ { | |||
|
|||
// Create a list of daemon types. The length is the total number of built-in provisioners, and |
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.
So, at a high level, provisioner daemons don't have a type. Provisioners have a type, and a provisioner daemon can accept multiple provisioner types via the Connector
map.
Do we really want/need to have a different number of daemons for echo and terraform? Or the choice really between: echo, terraform, or both? If so, maybe it makes sense to keep the flag about the number as is, and then add a flag (default true) that controls whether to include terraform provisioners.
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.
So, at a high level, provisioner daemons don't have a type. Provisioners have a type, and a provisioner daemon can accept multiple provisioner types via the Connector map.
Yes, but that assumes the provisioner implementation can support both types. Right now we have an echo provisioner daemon implementation, and a terraform. To unify those into a single daemon, we'd need a daemon implementation that can support both.
I think in practice, having provisioner daemons support 1 job type is reasonable if those jobs are unique enough. Example being if we add OpenTF as a new type, you could probably make the terraform
provisioner support both terraform
and open-tf
.
For the flag. The status quo is you can have either terraform provisioners xor echo provisioners. For e2e testing, it makes sense to have both, as the echo provisioner obviously has less overhead for some of our tests.
I think it makes sense to be able to control whether you run terraform only (prod), both (e2e), just echo (unit testing). So the flags need to support these 3 cases.
The --provisioner-daemons
flag is the flag our customers use today to control the number of built-in terraform provisioners. Given --provisioner-daemons-echo
is hidden and only used in testing, it feels reasonable to give it the same functionality as it's production counterpart, just for echo provisioners instead.
847090a
to
7defa52
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Value: serpent.Validate(&c.Provisioner.DaemonTypes, func(values *serpent.StringArray) error { | ||
if values == nil { | ||
return nil | ||
} | ||
|
||
for _, value := range *values { | ||
if err := ProvisionerTypeValid(value); err != nil { | ||
return err | ||
} | ||
} | ||
|
||
return nil | ||
}), |
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.
some enum validation. This is case sensitive.
75236a1
to
da51c9b
Compare
Built-in provisioners server their respective types, not both.
da51c9b
to
487f0a8
Compare
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.
LGTM
What this does
Allows us to spin up terraform & echo provisioners as built-ins. This is to support e2e testing using both terraform & echo for different tests.
Also changed echo provisioners to not accept terraform template version jobs. Before, these jobs would just hang anyway.
To launch both:
UI
Adds a UI element to the health page to indicate supported provisioner types for each provisioner.
Before
Before this
--provisioner-daemons-echo=true
was a boolean value and--provisioner-daemons
told the number of provisioners. So theprovisioner-daemons-echo
flag changes how the other flag works. Now they both work independently.