- Notifications
You must be signed in to change notification settings - Fork928
fix: fix null pointer on external provisioner daemons with daily_cost#9401
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
Uh oh!
There was an error while loading.Please reload this page.
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. |
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.
Tests seem to be failing, but I'm otherwise fine with the change (sans panics). The other suggestion re: args, feel free to consider or disregard.
| } | ||
| if deploymentValues == nil { | ||
| panic("deploymentValues is nil") | ||
| } |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 inNew (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
spikecurtis commentedAug 30, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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