- Notifications
You must be signed in to change notification settings - Fork928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 actuallyprovision anything. |
Uh oh!
There was an error while loading.Please reload this page.
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 theConnector map.
Do we really want/need to have a different number ofdaemons 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 theterraform provisioner support bothterraform andopen-tf.
For the flag. The status quo is you can have either terraform provisionersxor 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 to7defa52CompareEmyrk commentedMay 2, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 toda51c9bCompareBuilt-in provisioners server their respective types, not both.
da51c9b to487f0a8CompareThere 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
Uh oh!
There was an error while loading.Please reload this page.
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=truewas a boolean value and--provisioner-daemonstold the number of provisioners. So theprovisioner-daemons-echoflag changes how the other flag works. Now they both work independently.