-
Notifications
You must be signed in to change notification settings - Fork 978
chore: add organization_id column to provisioner daemons #12356
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
db381a4
to
c11cfbc
Compare
organization_id = (SELECT id FROM organizations ORDER BY organizations.created_at ASC LIMIT 1 ); | ||
|
||
ALTER TABLE provisioner_daemons | ||
ALTER COLUMN organization_id SET NOT NULL; |
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.
Do we still need to explicitly put this stuff in a transaction? Otherwise a concurrent insert of a provisioner daemon could cause this ALTER
to fail.
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.
No, by default all migrations run in a tx now.
provisioner_daemons | ||
SET | ||
-- Default to the first org | ||
organization_id = (SELECT id FROM organizations ORDER BY organizations.created_at ASC LIMIT 1 ); |
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.
it's an odd case, but this will fail if there is a provisioner daemon but no default org, which could happen if you start coderd on an older version (which creates in-memory provisioner daemons) but don't create the first user, and then migrate.
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.
Yes, we need this PR in first: #12412
@@ -1239,10 +1239,18 @@ func (api *API) CreateInMemoryProvisionerDaemon(dialCtx context.Context, name st | |||
} | |||
}() | |||
|
|||
// All in memory provisioners will be apart of the default org for now. | |||
//nolint:gocritic // in-memory provisioners are owned by system | |||
defaultOrg, err := api.Database.GetDefaultOrganization(dbauthz.AsSystemRestricted(dialCtx)) |
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 think you've already noticed that this will fail if the default organization hasn't been created. So think we need have a migration that creates a default org if none exists before we can have this.
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.
Yea, I will make sure this PR is merged first: #12412
Closes #11931