Skip to content

feat: Update CLI to handle managed variables #6220

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 63 commits into from
Feb 17, 2023
Merged

feat: Update CLI to handle managed variables #6220

merged 63 commits into from
Feb 17, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Feb 15, 2023 •
edited
Loading

Related: #5980

This PR extends the CLI commands create and push to let user pass the variables.yml file with Terraform variable values.

Extra changes:

  1. API /variables: redact value and default value for sensitive parameters.
  2. Fix: variable is marked as required if variable.default == nil not just variable.default == "".

Testing:

./scripts/coder-dev.sh templates create docker-rich-2 -d mtojek/docker-rich --variables-file values.yml

./scripts/coder-dev.sh templates push docker-2 -d mtojek/docker-rich --variables-file values.yml

./scripts/coder-dev.sh templates push docker-2 -d mtojek/docker-rich --variable aaa=bbb
  • Use variables file to pass initial values for the new template
  • Pull variable values from the active template, so that user doesn't have to pass them again
  • Write unit tests

@mtojek mtojek mentioned this pull request Feb 15, 2023
Closed
@mtojek mtojek self-assigned this Feb 15, 2023
@mtojek mtojek marked this pull request as ready for review February 16, 2023 13:39
@mtojek mtojek requested a review from kylecarbs February 16, 2023 13:42
kylecarbs
kylecarbs approved these changes Feb 16, 2023
View reviewed changes
Copy link
Member

@kylecarbs kylecarbs left a comment

Choose a reason for hiding this comment

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

Looks good.

I think users could confuse the help of variables-file and parameters-file, maybe that's something to look at before merge.

@mtojek
Copy link
Member Author

mtojek commented Feb 16, 2023

The original form:

Specify a file path with values for managed variables.

It's hard to figure out a good description as we don't invent anything new. These are just Terraform variables. A couple of ideas:

Specify a file path with values for Terraform-managed variables.

Specify a file path with Terraform variable values for the template.

Specify a file path with template-wide Terraform variables.

Any preference here, @kylecarbs @bpmct? The task will be harder once we replace the Terraform with something else.

@kylecarbs
Copy link
Member

kylecarbs commented Feb 16, 2023 •
edited
Loading

I like "Specify a file path with values for Terraform-managed variables."

bpmct
bpmct reviewed Feb 16, 2023
View reviewed changes
Copy link
Member

@bpmct bpmct left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Any plans to support something like TF_VAR=, -var, or --set as well?

In a CI pipeline when a new image is pushed, how could we set that output (ami-21912383) as a variable when a Coder template update is also pushed as the next step in the pipeline? Writing to a file and then referencing is a strange workaround.

@mtojek
Copy link
Member Author

mtojek commented Feb 16, 2023 •
edited
Loading

Writing to a file and then referencing is a strange workaround.

Yes, it depends. If you have multiple variables you need to reference, then a file would be more convenient. Same with sensitive values, you don't want somebody to sniff them via ps aux.

Anyway, I can add an extra command option (--variable "name=value") to pass more variables, but I wouldn't limit it to TF_VAR in case we'd like to support different provisioners. What do you think?

@bpmct
Copy link
Member

bpmct commented Feb 16, 2023

Anyway, I can add an extra command option (--variable "name=value") to pass more variables, but I wouldn't limit it to TF_VAR in case we'd like to support different provisioners. What do you think?

Sounds fair 👍🏼

@mtojek mtojek requested review from bpmct February 16, 2023 19:27
@mtojek
Copy link
Member Author

mtojek commented Feb 16, 2023

Done!

@mtojek mtojek merged commit a69137b into coder:main Feb 17, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kylecarbs kylecarbs kylecarbs approved these changes

@bpmct bpmct Awaiting requested review from bpmct

Assignees

@mtojek mtojek

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mtojek @kylecarbs @bpmct