Skip to content

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.

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

Merged
merged 5 commits into from
Aug 30, 2023
Merged

fix: fix null pointer on external provisioner daemons with daily_cost #9401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac192e7
fix: fix null pointer on external provisioner daemons with daily_cost
spikecurtis Aug 29, 2023
3c0d020
Add logging for debounce and job acquire
spikecurtis Aug 29, 2023
a91d28f
Return error instead of panic
spikecurtis Aug 30, 2023
8e32e5c
remove debounce on external provisioners to fix test flakes
spikecurtis Aug 30, 2023
8956184
Merge branch 'main' of https://github.com/coder/coder into spike/8785…
spikecurtis Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 25 additions & 20 deletions coderd/coderd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1098,26 +1098,31 @@ func (api *API) CreateInMemoryProvisionerDaemon(ctx context.Context, debounce ti
}

mux := drpcmux.New()

err = proto.DRPCRegisterProvisionerDaemon(mux, &provisionerdserver.Server{
AccessURL: api.AccessURL,
ID: daemon.ID,
OIDCConfig: api.OIDCConfig,
Database: api.Database,
Pubsub: api.Pubsub,
Provisioners: daemon.Provisioners,
GitAuthConfigs: api.GitAuthConfigs,
Telemetry: api.Telemetry,
Tracer: tracer,
Tags: tags,
QuotaCommitter: &api.QuotaCommitter,
Auditor: &api.Auditor,
TemplateScheduleStore: api.TemplateScheduleStore,
UserQuietHoursScheduleStore: api.UserQuietHoursScheduleStore,
AcquireJobDebounce: debounce,
Logger: api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)),
DeploymentValues: api.DeploymentValues,
})
srv, err := provisionerdserver.NewServer(
api.AccessURL,
daemon.ID,
api.Logger.Named(fmt.Sprintf("provisionerd-%s", daemon.Name)),
daemon.Provisioners,
tags,
api.Database,
api.Pubsub,
api.Telemetry,
tracer,
&api.QuotaCommitter,
&api.Auditor,
api.TemplateScheduleStore,
api.UserQuietHoursScheduleStore,
api.DeploymentValues,
debounce,
Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Member

@mafredri mafredri Aug 29, 2023 •
edited
Loading

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 in New (like some args are now). IMO that levels the playing field.

provisionerdserver.Options{
OIDCConfig: api.OIDCConfig,
GitAuthConfigs: api.GitAuthConfigs,
},
)
if err != nil {
return nil, err
}
err = proto.DRPCRegisterProvisionerDaemon(mux, srv)
if err != nil {
return nil, err
}
Expand Down
Loading