Skip to content

chore(examples): remove deprecated startup_script_timeout #12104

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 3 commits into from
Feb 12, 2024
Merged

chore(examples): remove deprecated startup_script_timeout #12104

merged 3 commits into from
Feb 12, 2024

Conversation

johnstcn
Copy link
Member

@johnstcn johnstcn commented Feb 12, 2024 •
edited
Loading

Creating a template from examples results in deprecation warnings, which is a bit of a product smell from a user's perspective.

@johnstcn johnstcn self-assigned this Feb 12, 2024
@johnstcn johnstcn requested review from matifali and mtojek February 12, 2024 13:46
matifali
matifali reviewed Feb 12, 2024
View reviewed changes
Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

What is the alternative of this deprecated timeout flags?

@johnstcn @matifali
Update scaletest/templates/scaletest-runner/main.tf
054265b 
Co-authored-by: Muhammad Atif Ali <atif@coder.com>
@johnstcn
Copy link
Member Author

johnstcn commented Feb 12, 2024 •
edited
Loading

What is the alternative of this deprecated timeout flags?

@mafredri feel free to correct me if I'm wrong, but I believe startup_script_behavior is the replacement: https://registry.terraform.io/providers/coder/coder/latest/docs/resources/agent#startup_script_behavior

edit: see below

@mafredri
Copy link
Member

What is the alternative of this deprecated timeout flags?

@mafredri feel free to correct me if I'm wrong, but I believe startup_script_behavior is the replacement: registry.terraform.io/providers/coder/coder/latest/docs/resources/agent#startup_script_behavior

There is no replacement, actually. And startup_script_behavior is kind of deprecated too (as per updated provider docs), both startup_script and startup_script_behavior are simply aliases for defining a coder_script with certain properties.

@matifali
Copy link
Member

matifali commented Feb 12, 2024 •
edited
Loading

It is a bit out of scope, but thoughts on replacing all arch and os attributes for the coder_agent resource with data.coder_workspace.me.arch and data.coder_workspace.me.os. Currently, I see a mix of both. We can do it in a separate PR, but we want your thoughts.

One downside of using it could be a failed workspace when workspace compute and provisionerd have different os or arch.

@johnstcn
Copy link
Member Author

A bit out of scope but thoughts on replacing all arch and os attributes for the coder_agent resource with darta.coder_workspace.me.arch and data.coder_workspace.me.os. Currently, I see a mix of both. We can do it in a separate PR, but we would like to hear your thoughts.

One downside of using it could be when workspace compute and provisionerd have different os or arch.

Definitely something for a separate PR, but I think the case you mention is solvable with a comment above the setting.

mtojek
mtojek approved these changes Feb 12, 2024
View reviewed changes
Copy link
Member

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

👍

@matifali
Copy link
Member

What is the alternative of this deprecated timeout flags?

@mafredri feel free to correct me if I'm wrong, but I believe startup_script_behavior is the replacement: registry.terraform.io/providers/coder/coder/latest/docs/resources/agent#startup_script_behavior

There is no replacement, actually. And startup_script_behavior is kind of deprecated too (as per updated provider docs), both startup_script and startup_script_behavior are simply aliases for defining a coder_script with certain properties.

We should then also remove startup_script_behaviour.

matifali
matifali reviewed Feb 12, 2024
View reviewed changes
Copy link
Member

@matifali matifali left a comment

Choose a reason for hiding this comment

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

a few more suggestions.

@johnstcn @matifali
Update scaletest/terraform/k8s/coder.tf
a22a186 
Co-authored-by: Muhammad Atif Ali <atif@coder.com>
@mafredri
Copy link
Member

We should then also remove startup_script_behaviour.

We could, but then we must rewrite all startup_script entries as coder_script resources with the matching blocking setting.

@johnstcn johnstcn merged commit 1cc51b0 into main Feb 12, 2024
@johnstcn johnstcn deleted the cj/rm-startup-script-timeout branch February 12, 2024 14:29
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@matifali matifali matifali approved these changes

@mafredri mafredri mafredri approved these changes

@mtojek mtojek mtojek approved these changes

Assignees

@johnstcn johnstcn

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@johnstcn @mafredri @matifali @mtojek