fix: fix null pointer on external provisioner daemons with daily_cost #9401
Conversation
Signed-off-by: Spike Curtis <spike@coder.com>
|
I did a little refactor on the provisionerdserver so that it's explicit about what arguments are required and what are optional, so that we're less likely to pass null pointers by omission. |
| } | ||
| if deploymentValues == nil { | ||
| panic("deploymentValues is nil") | ||
| } |
There was a problem hiding this comment.
I would rather we return errors here so this can be handled more gracefully at the calling site.
Let's say we introduce a regression where, in a certain configuration, some of these are passed as nil. So our tests don't catch it, but our customer will. By the time these panic, coderd could've started some work that will be canceled abruptly (hypothetically, probably not the way todays server starts up, but in the future).
| api.TemplateScheduleStore, | ||
| api.UserQuietHoursScheduleStore, | ||
| api.DeploymentValues, | ||
| debounce, |
There was a problem hiding this comment.
This feels like a lot of args and has it's own risk of mixed up arg order when there are same types (not the case here, now). Typically I'd revert to a struct but I understand why we're making this change. Two structs may be another option. We have a linter that checks that all fields are set (only enabled for certain types atm).
There was a problem hiding this comment.
The "a lot of args" is it's own code smell, and is a symptom of poor code architecture to have so many required args.
Making it a struct makes it too easy to leave off required arguments. It doesn't reduce the amount of code, and in fact, increases it.
There was a problem hiding this comment.
Making it a struct makes it too easy to leave off required arguments. It doesn't reduce the amount of code, and in fact, increases it.
I think we should value clarity more-so than reducing the number of characters. And it's not like it would increase LOC, just verbosity (but also clarity). But again, feel free to take this as a suggestion, not as an asked change.
PS. I was making a case for a linter that detects missing fields. And the struct fields can be further verified in New (like some args are now). IMO that levels the playing field.
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
Signed-off-by: Spike Curtis <spike@coder.com>
…-daily-cost-ext-provisioner-daemon
|
I've disabled "debounce" in external provisioners. This matches the status quo, but is still a problem, because lots of external provisioners can create a large query burden on the DB. Tracking that here: #9428 Setting debounce was causing test flakes because debounce relies on global state and thus tests were getting debounced by one another. |
fixes #8785