- Notifications
You must be signed in to change notification settings - Fork1k
fix: adjust workspace claims to be initiated by users#20179
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
base:main
Are you sure you want to change the base?
Conversation
…e the prebuilds system
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 like this change, it makes sense for the initiator to be the user who actually claims the prebuild rather than the system user 👍
I only have one concern: after this change, existing data in the database will have mixed information. Olderworkspace_build rows will have the prebuilds system user as the initiator, while new ones will have the user who claimed the workspace. This means the data becomes inconsistent in how the initiator is represented.
I don’t think this will be an issue in practice, but it’s definitely something we should document in case we add future database queries or analytics that rely on this field. Maybe a short code comment or note in the schema would help make that clear?
I can see an argument for creating a migration to update the existing schema, as the initiator ID shows up in the following tables:
I'd suggest keeping it out of scope of this PR. The fact that this change hasn't requiredany updates to existing tests is mildly concerning though. |
Agree 💯 I don't think we test the initiator ID 😕 so it might actually be a good idea to also add a test to cover this new change! |
We have a thorough test suite on prebuild claiming already. |
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.
LGTM 👍 Thank you for adding the tests.
Regarding the possible migration to update the existing schema, we can do that on a follow-up PR or create a separate issue.
Uh oh!
There was an error while loading.Please reload this page.
The prebuilds user never initiates a workspace claim autonomously. A claim can only happen when a user attempts to create a workspace. When listing prebuild provisioner jobs, it would not make sense to see jobs related to users who are creating workspaces and have gotten a prebuilt workspace. When cleaning up an overwhelmed provisioner queue, we should not delete claims as they have humans waiting for them and are not part of the thundering herd.
Therefore, this PR ensures that provisioner jobs that claim workspaces are considered to be initiated by the user, not the prebuilds system.