Navigation Menu

Skip to content
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: import value from legacy variable to build parameter #6556

Merged
merged 20 commits into from Mar 14, 2023

Conversation

mtojek
Copy link
Member

@mtojek mtojek commented Mar 10, 2023

Resolves: #6368
Resolves: #6074

This PR extends the "build workspace" logic to pull values from legacy variables to make the parameter migration smooth.

I added a relatively long unit test to cover the whole migration path (pulling value from legacy parameter).

@mtojek mtojek self-assigned this Mar 10, 2023
@aaronlehmann
Copy link
Contributor

Is there a supported migration path for list(string) variables, or is it best to create a new string parameter (for example, CSV-formatted) and support both the legacy variable and the new parameter during the migration period?

@mtojek
Copy link
Member Author

mtojek commented Mar 13, 2023

Is there a supported migration path for list(string) variables, or is it best to create a new string parameter (for example, CSV-formatted)

So far we don't plan to add more types to coder_parameter, so you may want to "downgrade" to an ordinary string. BTW if you find it useful, please open an issue, describe your use case, and we can review it together.

support both the legacy variable and the new parameter during the migration period?

I will improve docs here, and describe the recommended workflow as part of #6074. Is this parameter intended to be modified by users? Maybe you want to rather keep it as managed Terraform variable.

@mtojek mtojek marked this pull request as ready for review March 13, 2023 13:03
@aaronlehmann
Copy link
Contributor

So far we don't plan to add more types to coder_parameter, so you may want to "downgrade" to an ordinary string. BTW if you find it useful, please open an issue, describe your use case, and we can review it together.

The existing variable is used to specify a free-form list of security groups for the workspace. I had planned to convert this to a comma-separated string when I transition to rich parameters, but I'm a little unclear on the migration path - it sounds I will need my own logic to pull the value from either the coder_parameter or the TF variable, as applicable.

I will improve docs here, and describe the recommended workflow as part of #6074. Is this parameter intended to be modified by users? Maybe you want to rather keep it as managed Terraform variable.

Yes, the parameter is for users to modify. I thought that support for legacy variables was planned to be removed in April?

@mtojek
Copy link
Member Author

mtojek commented Mar 13, 2023

The existing variable is used to specify a free-form list of security groups for the workspace.

Well, so far we can support string + single choice options, bool, and number types.

but I'm a little unclear on the migration path - it sounds I will need my own logic to pull the value from either the coder_parameter or the TF variable, as applicable.

I described the migration steps here. As you can see, you can configure legacy_variable_name and legacy_variable to point to the legacy parameter, and make the migration smoother.

Yes, the parameter is for users to modify. I thought that support for legacy variables was planned to be removed in April?

That is correct, but it doesn't mean that we will remove variables from Terraform templates. Those will be still available to use via managed Terraform variables (see: #5980).

@aaronlehmann
Copy link
Contributor

but I'm a little unclear on the migration path - it sounds I will need my own logic to pull the value from either the coder_parameter or the TF variable, as applicable.

I described the migration steps here. As you can see, you can configure legacy_variable_name and legacy_variable to point to the legacy parameter, and make the migration smoother.

Right, but I'm assuming that won't work for a list(string) legacy variable. Is that incorrect?

Yes, the parameter is for users to modify. I thought that support for legacy variables was planned to be removed in April?

That is correct, but it doesn't mean that we will remove variables from Terraform templates. Those will be still available to use via managed Terraform variables (see: #5980).

The link says "Workspace users are not able to modify template variables", so it sounds ilke I will need to convert this to a coder_parameter one way or another.

Copy link
Member

@mafredri mafredri 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 👍🏻, mainly had some feedback on the docs language.

Comment on lines 224 to 242
1. Prepare and update a new template version:

- Add `coder_parameter` resource matching the legacy parameter to migrate.
- Use `legacy_variable_name` and `legacy_variable` to link both.
- Mark the new parameter as `mutable`, so that Coder will not block updating existing workspaces.

2. Update all workspaces to the uploaded template version. Coder will populate `coder_parameter`s with values from legacy parameters.
3. Prepare another template version:

- Remove migrated variable.
- Remove properties `legacy_variable` and `legacy_variable_name` from `coder_parameter`s.

4. Update all workspaces to the uploaded template version.
5. Prepare another template version:

- Enable the `feature_use_managed_variables` provider flag to use managed Terraform variables for template customization. Once the flag is enabled, legacy parameters won't be used.

6. Update all workspaces to the uploaded template version.
7. Delete legacy parameters.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. Prepare and update a new template version:
- Add `coder_parameter` resource matching the legacy parameter to migrate.
- Use `legacy_variable_name` and `legacy_variable` to link both.
- Mark the new parameter as `mutable`, so that Coder will not block updating existing workspaces.
2. Update all workspaces to the uploaded template version. Coder will populate `coder_parameter`s with values from legacy parameters.
3. Prepare another template version:
- Remove migrated variable.
- Remove properties `legacy_variable` and `legacy_variable_name` from `coder_parameter`s.
4. Update all workspaces to the uploaded template version.
5. Prepare another template version:
- Enable the `feature_use_managed_variables` provider flag to use managed Terraform variables for template customization. Once the flag is enabled, legacy parameters won't be used.
6. Update all workspaces to the uploaded template version.
7. Delete legacy parameters.
1. Prepare and update a new template version:
- Add `coder_parameter` resource matching the legacy parameter to migrate.
- Use `legacy_variable_name` and `legacy_variable` to link both.
- Mark the new parameter as `mutable`, so that Coder will not block updating existing workspaces.
2. Update all workspaces to the uploaded template version. Coder will populate `coder_parameter`s with values from legacy parameters.
3. Prepare another template version:
- Remove migrated variable.
- Remove properties `legacy_variable` and `legacy_variable_name` from `coder_parameter`s.
4. Update all workspaces to the uploaded template version.
5. Prepare another template version:
- Enable the `feature_use_managed_variables` provider flag to use managed Terraform variables for template customization. Once the flag is enabled, legacy parameters won't be used.
6. Update all workspaces to the uploaded template version.
7. Delete legacy parameters.

I think this might be a bit easier to follow (indented bullet points).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
docs/templates/parameters.md Outdated Show resolved Hide resolved
mtojek and others added 9 commits March 14, 2023 10:56
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
@mtojek mtojek enabled auto-merge (squash) March 14, 2023 10:07
@mtojek mtojek merged commit 7587850 into coder:main Mar 14, 2023
17 of 19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rich parameters: import values from legacy parameters docs: rich parameters
3 participants