Skip to content

feat: Read params from file for template/workspace creation #1541

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

Merged
merged 13 commits into from
May 20, 2022

Conversation

AbhineetJain
Copy link
Contributor

@AbhineetJain AbhineetJain commented May 18, 2022

This allows users to use parameter files for variable input during template and workspace creation.

Subtasks

  • read parameters from a yaml file for workspace creation
  • read parameters from a yaml file for template creation
  • added tests for feature

Fixes #1377

@AbhineetJain AbhineetJain changed the title Read params from file for template/workspace creation feat: Read params from file for template/workspace creation May 18, 2022
@@ -18,6 +20,7 @@ func create() *cobra.Command {
var (
workspaceName string
templateName string
parameterFile string
Copy link
Member

Choose a reason for hiding this comment

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

tfvars is the standard way to handle this with Terraform.

I'd prefer to not have our own solution, because that'd remove the ability for users to do terraform apply.

Copy link
Member

Choose a reason for hiding this comment

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

@kylecarbs that assumes we use terraform, but our cli should be terraform agnostic right?

Also tfvars is just HCL or yaml. So we could just support multiple file types

Copy link
Member

Choose a reason for hiding this comment

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

Developers also won't have the tf file when running coder workspace create, only the person who made the template will have the tf file.

Copy link
Member

Choose a reason for hiding this comment

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

We can be Terraform agnostic while still consuming tfvars. Latching onto the existing Terraform ecosystem is a much better user experience than forking it with our own parameter format.

I don't believe it's necessary to have a tfvars equivalent when creating a workspace. Or at least, we haven't seen it as a problem yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue #1377 mentions creating workspaces using parameter files, which is why I went with a non-tfvars approach. However, #1426 does talk about doing this for templates. Would you recommend solving both problems using different approaches or only work on #1426 (template creation via file) via tfvars? Let me quickly check how feasible using tfvars for template creation is.

Copy link
Contributor Author

@AbhineetJain AbhineetJain May 18, 2022

Choose a reason for hiding this comment

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

Is there a documented flow of how the customers would use terraform apply when setting up templates? I tried using the tfvars file as follows:

$ terraform init
$ terraform apply -var-file="<path_to_tfvars>"
...
Apply complete! Resources: 3 added, 0 changed, 0 destroyed.

It did not create a template and calling coder templates create after this still asked me for input, so I am not sure what the actual flow should be.

@AbhineetJain AbhineetJain marked this pull request as ready for review May 20, 2022 04:16
@AbhineetJain
Copy link
Contributor Author

Interestingly, the error I am encountering in the Windows tests is a known Go issue: golang/go#52986. Trying a workaround.

@AbhineetJain
Copy link
Contributor Author

AbhineetJain commented May 20, 2022

The fix does not help. What is the precedent in these cases? Revert the implementation fix and skip the tests until Go fixes this?

@AbhineetJain AbhineetJain force-pushed the 1377-feat-load-parameters-from-file branch from 86b1028 to 1491620 Compare May 20, 2022 14:40
@AbhineetJain AbhineetJain merged commit 7c3e1a5 into main May 20, 2022
@AbhineetJain AbhineetJain deleted the 1377-feat-load-parameters-from-file branch May 20, 2022 15:29
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
* Read params from file for template/workspace creation

* Use os.ReadFile

* Refactor reading params into a separate module

* Add comments and unit tests

* Rename variable

* Uncomment and fix unit test

* Fix comment

* Refactor tests

* Fix unit tests for windows

* Fix unit tests for Windows

* Add comments for the hotfix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: load parameters from file on workspace creation
3 participants