- Notifications
You must be signed in to change notification settings - Fork23
Replacelogin_before_ready withstartup_script_behavior#125
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
Replacelogin_before_ready withstartup_script_behavior#125
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fbd5cd1 todb53750Comparedb53750 to609d6e0Compare| // the user setting this or "login_before_ready". For all | ||
| // intents and purposes, until deprecation, setting | ||
| // "login_before_ready = false" is equivalent to setting | ||
| // "startup_script_behavior = blocking". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We will handle this case in coder/coder, ifstartup_script_behavior is empty string, we take the value fromlogin_before_ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nothing blocking 👍
provider/agent_test.go Outdated
| shutdown_script = "echo bye bye" | ||
| shutdown_script_timeout = 120 | ||
| login_before_ready =false | ||
| startup_script_behavior ="blocking" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: can we add some test cases whenstartup_script_behavior is non-blocking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added a bunch of test cases, although since there's no logic now it's a bit silly. 😄
Uh oh!
There was an error while loading.Please reload this page.
| return&schema.Resource{ | ||
| Description:"Use this resource to associate an agent.", | ||
| CreateContext:func(c context.Context,resourceData*schema.ResourceData,iinterface{}) diag.Diagnostics { | ||
| CreateContext:func(ctx context.Context,resourceData*schema.ResourceData,iinterface{}) diag.Diagnostics { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: I'm curious if there is a way to check ifCheckContext calledupdateStartupScriptBehaviorIfLoginBeforeReady or if it wasReadWithoutTimeout.
BTW It would be nice to write a comment on why we need to call it in both contexts. I think that we usually stick withReadContext.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Both are called, but I never got this method working because the value reverts at the end resulting inStep 1/1 error: After applying this test step, the plan was not empty..
This function was actually supposed to be removed in favor of empty default forstartup_script_behavior, so it's good you asked.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Fixes#124