Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat!: drop support for legacy parameters#7663

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

Merged
mtojek merged 47 commits intocoder:mainfrommtojek:7452-remove-legacy-parameters
Jun 2, 2023

Conversation

mtojek
Copy link
Member

@mtojekmtojek commentedMay 24, 2023
edited
Loading

Fixes:#7452

This PR rips off legacy parameters from coder source code. The size of the PR is huge, it is basically unreviewable, so I suggest playing around with the branch.

I removed references to legacy parameters from:

  • API/coderd
  • provisioners and provisioner daemon
  • CLI
  • site/UI

Once this is merged, coder will requirefeature_use_managed_variables = true if there any variables. This is the only way we can distinguish if variable is potentially a legacy parameter. After a few releases, we can remove that requirement.

I didn't remove the relevant database table in case somebody would like to roll back to the previous release.

Screenshot 2023-05-25 at 13 25 06

Extra changes:

  • API methods with empty responses (deprecated endpoint)d6e5207
  • Extra check to block pushing templates with legacy parameters.0ed1bad
  • Legacy parameters verification8c5eee9

@mtojekmtojek self-assigned thisMay 24, 2023
@mtojekmtojekforce-pushed the7452-remove-legacy-parameters branch from884dbbc to70b2a98CompareMay 24, 2023 17:21
@matifali
Copy link
Member

Can we use terraform variable withuse_managed_variable set in the coder terraform provider?

@mtojek
Copy link
MemberAuthor

Can we use terraform variable with use_managed_variable set in the coder terraform provider?

For detecting pushed templates with legacy params? Yes, that's the assumption 👍

@matifali
Copy link
Member

Can we use terraform variable with use_managed_variable set in the coder terraform provider?

For detecting pushed templates with legacy params? Yes, that's the assumption 👍

I mean after the deprecation. Can we still use them to store sensitive data?

@mtojek
Copy link
MemberAuthor

I mean after the deprecation. Can we still use them to store sensitive data?

After deprecation, users will be able to use Terraform variables only asmanaged ones.

matifali reacted with thumbs up emoji

@mtojekmtojekforce-pushed the7452-remove-legacy-parameters branch fromf7c703e tod6e5207CompareMay 25, 2023 10:47
@mtojekmtojekforce-pushed the7452-remove-legacy-parameters branch from8915f75 to217ea69CompareMay 25, 2023 11:08
@mtojekmtojek marked this pull request as ready for reviewMay 25, 2023 13:07
@mtojekmtojek requested a review fromjohnstcnMay 25, 2023 13:08
Copy link
Collaborator

@BrunoQuaresmaBrunoQuaresma left a comment

Choose a reason for hiding this comment

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

LGTM

@mtojek
Copy link
MemberAuthor

add a permanent banner highlighting that the workspace with legacy parameters is not supported

Screenshot 2023-05-31 at 18 18 47Screenshot 2023-05-31 at 18 17 01

show warning in the CLI

Screenshot 2023-05-31 at 17 52 50

try to find a way to delete the impaired workspace if it requires Terraform variables to do that

  1. Template author should addfeature_use_managed_variables = true to the template.
  2. Update the affected workspace.
  3. Delete the workspace.

Clean!

@mtojekmtojek requested a review fromspikecurtisMay 31, 2023 16:19
@bpmct
Copy link
Member

Awesome!! How will the template author know to do steps 1-3? Are there cases where deleting the workspace will not work until steps 1-3 are completed?

Perhaps we should link to some docs on how to clean up the old workspaces in the message.

@ammarioammario changed the titlefeat!: Drop support for legacy parametersfeat!: drop support for legacy parametersMay 31, 2023
@spikecurtis
Copy link
Contributor

try to find a way to delete the impaired workspace if it requires Terraform variables to do that

  1. Template author should addfeature_use_managed_variables = true to the template.
  2. Update the affected workspace.
  3. Delete the workspace.

Clean!

  1. I have a workspace up that uses legacy parameters
  2. Upgrade Coderd
  3. Stop the workspace

I get failed stop

Error: No value for required variableon main.tf line 27:  27: variable "legacy" {The root module input variable "legacy" is not set, and has no default value. Use a -var or -var-file command line argument to provide a value for this variable.

But, then, if I

  1. Update the template to convert to a rich parameter, setfeature_use_managed_variables = true
  2. Update the workspace (it prompts me for the parameter value --- no migration of my old value)

It succeeds building.

A little gnarly, but works out ok in the end.@bpmct you ok with that?

mtojek reacted with thumbs up emoji

@spikecurtis
Copy link
Contributor

So this UX is confusing:

Screen.Recording.2023-06-01.at.2.10.24.PM.mov

If I try to change to an old version with legacy parameters, it

a) doesn't change the version
b) tells me to delete the workspace

The warning box that comes up should say something like, "Unable to change version. Legacy parameters in use on this version aren't supported any more."

@spikecurtis
Copy link
Contributor

The error message says, "Legacy parameters are not supported anymore, delete the workspace and the related template."

This isn't great advice

If you do delete the workspace, the delete job will always fail because there are variables in the template that we aren't supplying value to. It then tells you to have an admin delete with--orphan, but we could skip straight to this, instead of having the user bang their head against a failed delete.

Also, deleting the template is not necessary. The template author can just rework the variables to "coder_parameters", push a new version, and then people can upgrade.

Basically, there is nothing a regular (non-template-admin, non-owner) user can meaningfully do if they have a workspace that uses legacy parameters after upgrade. Our advice to end users should just be, "Contact your administrator for assistance."

Our advice to admins (this can be in docs & release notes, it doesn't have to be in the dashboard IMO) should be:

Decide whether you are going to fix the template to use rich parameters. If you aren't going to fix it, then delete all workspaces with--orphan, manually clean up infrastructure resources, and delete the template. If you are going to fix it, then push a new version, and upgrade all the affected workspaces.

@mtojek
Copy link
MemberAuthor

The warning box that comes up should say something like, "Unable to change version. Legacy parameters in use on this version aren't supported any more."

It is one more API to cover with extra condition, should not be hard to do this.

Despite that, I think that we should not invest much time and efforts on making the whole operation really smooth. In theory, this is already beyond the deprecation timeline (end of April), so most of the users should be already migrated. I doubt that these paths will be actively followed.

Basically, there is nothing a regular (non-template-admin, non-owner) user can meaningfully do if they have a workspace that uses legacy parameters after upgrade

Thanks for providing UX feedback. "Contact your administrator for assistance." seems to be a better fit and actionable for every regular user. I also like the idea to describe admin steps in docs. Happy to adjust both.

@mtojek
Copy link
MemberAuthor

mtojek commentedJun 1, 2023
edited
Loading

It is one more API to cover with extra condition, should not be hard to do this.

Actually it is more challenging that I expected, as "Change Version" dialog uses the same API method as Start or Stop workspace(/builds). If it isn't required, I wouldn't like to add more UI logic to handle this case, as this is contrary to what we're trying to achieve - remove references to legacy stuff.

I decided to just adjust wording here. Let me know your thoughts:

Screenshot 2023-06-01 at 16 11 44

EDIT:

Maybe it is better to keep docs aside from this PR as it is huge and hard to review. I will push the docs update in the follow-up as I wouldn't like to keep this submission open for a long time.

@mtojekmtojek requested a review fromspikecurtisJune 1, 2023 14:23
@mtojekmtojek marked this pull request as ready for reviewJune 1, 2023 14:23
Copy link
Contributor

@spikecurtisspikecurtis left a comment

Choose a reason for hiding this comment

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

I like the new error message and I agree that if it's a huge pain to fix up the Change Versions case, then we can live with some ugliness since it'll only affect people in the short term.

matifali reacted with thumbs up emoji
@mtojekmtojek merged commita7366a8 intocoder:mainJun 2, 2023
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJun 2, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@BrunoQuaresmaBrunoQuaresmaBrunoQuaresma approved these changes

@spikecurtisspikecurtisspikecurtis approved these changes

@bpmctbpmctAwaiting requested review from bpmct

@johnstcnjohnstcnAwaiting requested review from johnstcnjohnstcn is a code owner

Assignees

@mtojekmtojek

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deprecation plan for legacy parameters

5 participants

@mtojek@matifali@bpmct@spikecurtis@BrunoQuaresma

[8]ページ先頭

©2009-2025 Movatter.jp