feat(coderd): insert provisioner daemons #11207
Conversation
ee108ad to
02b6743
Compare
876ee58 to
b7bb176
Compare
| q.provisionerDaemons[idx].LastSeenAt = arg.LastSeenAt | ||
| return nil | ||
| } | ||
| return sql.ErrNoRows |
There was a problem hiding this comment.
Is this really the same return signature the DB gives? I believe you have to check the affected rows of the Exec as it will not return an error, just zero affected rows.
I wonder if we rely on similar assumptions elsewhere in the code, seeing as the generated code doesn't check affected rows 🤔
There was a problem hiding this comment.
Other dbmem queries have very similar logic; I just copied it.
There was a problem hiding this comment.
Ideally we should change it so that our dbmem logic matches reality. Otherwise a developer may rely on this behavior and wonder why it doesn't work in the real world.
There was a problem hiding this comment.
I'll file a follow-up issue 👍
EDIT: #11263
…nstraint triggers
| // Otherwise, the "owner" tag is always an empty string. | ||
| // NOTE: "owner" must NEVER be nil. Otherwise it will end up being | ||
| // duplicated in the database, as idx_provisioner_daemons_name_owner_key | ||
| // is a partial unique index that includes a JSON field. | ||
| func MutateTags(userID uuid.UUID, tags map[string]string) map[string]string { | ||
| if tags == nil { | ||
| tags = map[string]string{} | ||
| } | ||
| _, ok := tags[TagScope] | ||
| if !ok { | ||
| tags[TagScope] = ScopeOrganization | ||
| delete(tags, TagOwner) | ||
| tags[TagOwner] = "" | ||
| } | ||
| switch tags[TagScope] { | ||
| case ScopeUser: | ||
| tags[TagOwner] = userID.String() | ||
| case ScopeOrganization: | ||
| delete(tags, TagOwner) | ||
| tags[TagOwner] = "" | ||
| default: | ||
| tags[TagScope] = ScopeOrganization | ||
| tags[TagOwner] = "" |
There was a problem hiding this comment.
review: calling this out here -- I had assumed that the index over (name, tags["owner"]) would disallow multiple rows with ("foo", NULL) but this is apparently not the case. However, it does disallow multiple rows with ("foo", "").
I updated to use MutateProvisionerTags wherever possible when inserting a provisioner daemon.
There was a problem hiding this comment.
@mafredri helpfully pointed out that we can use COALESCE in the index to ensure that an absent value is treated the same as an empty string. 🎉 Adding a migration for this now.
| api.AccessURL, | ||
| id, | ||
| daemon.ID, |
There was a problem hiding this comment.
review: another call-out: I am now setting the ID of the provisionerd server to be the same as the provisioner daemon that was inserted into the database, instead of a random UUID as it was before.
I believe this should be OK, but deferring to better judgement here.
| q.provisionerDaemons[idx].LastSeenAt = arg.LastSeenAt | ||
| return nil | ||
| } | ||
| return sql.ErrNoRows |
There was a problem hiding this comment.
Ideally we should change it so that our dbmem logic matches reality. Otherwise a developer may rely on this behavior and wonder why it doesn't work in the real world.
| USING btree (name, lower(coalesce(tags->>'owner', '')::text)); | ||
|
|
||
| COMMENT ON INDEX idx_provisioner_daemons_name_owner_key | ||
| IS 'Allow unique provisioner daemon names by user'; |
There was a problem hiding this comment.
This makes it much more clear what it's purpose is ❤️
| //nolint:gocritic // This is specifically for updating the last seen at timestamp. | ||
| return db.UpdateProvisionerDaemonLastSeenAt(dbauthz.AsSystemRestricted(hbCtx), database.UpdateProvisionerDaemonLastSeenAtParams{ | ||
| ID: id, | ||
| LastSeenAt: sql.NullTime{Time: time.Now(), Valid: true}, |
There was a problem hiding this comment.
Noticed this was s.timeNow(), but turned into time.Now(). I suppose that's OK? Should it be dbtime.Now(), though?
There was a problem hiding this comment.
Should be s.timeNow, will hotfix 👍 Thanks for the catch
Relates to #10676