- Notifications
You must be signed in to change notification settings - Fork1k
feat: claim prebuilds based on workspace parameters instead of preset id#19279
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
e999a58 tof07ccccCompareThere 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.
Self Review
| } | ||
| func (q*querier)FindMatchingPresetID(ctx context.Context,arg database.FindMatchingPresetIDParams) (uuid.UUID,error) { | ||
| _,err:=q.GetTemplateVersionByID(ctx,arg.TemplateVersionID) |
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 don't like this, but I can't find a better way and there is precedent for this approach. Open to alternatives.
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 alternative is to essentially duplicate the logic inGetTemplateVersionByID, which I'm less a fan of.
| returnerr | ||
| } | ||
| ifb.templateVersionPresetID==uuid.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.
This is not strictly necessary for when a prebuilt workspace is claimed, but it ensures that a workspace build always has a preset ID set if one matches.
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 look good to me, I'm just slightly worried about the current policy returning the prebuild with the most specific set of parameters.
| } | ||
| func (q*querier)FindMatchingPresetID(ctx context.Context,arg database.FindMatchingPresetIDParams) (uuid.UUID,error) { | ||
| _,err:=q.GetTemplateVersionByID(ctx,arg.TemplateVersionID) |
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 alternative is to essentially duplicate the logic inGetTemplateVersionByID, which I'm less a fan of.
Uh oh!
There was an error while loading.Please reload this page.
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.
Minor nits on the query side, otherwise LGTM.
| SELECT unnest(@parameter_names::text[])AS name, | ||
| unnest(@parameter_values::text[])AS value |
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.
| SELECT unnest(@parameter_names::text[])AS name, | |
| unnest(@parameter_values::text[])AS value | |
| SELECT | |
| unnest(@parameter_names::text[])AS name, | |
| unnest(@parameter_values::text[])AS value |
I suggest avoiding alignment, and if needed, line-break so indentation can be used.
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.
done.
| COALESCE(COUNT(tvpp.name),0)AS total_preset_params, | ||
| COALESCE(COUNT(pp.name),0)AS matching_params | ||
| FROM template_version_presets tvp | ||
| LEFT JOIN template_version_preset_parameters tvppONtvpp.template_version_preset_id=tvp.id |
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.
These left join suggests there is merit in including presets with no parameters as well as zero matches with provided params, why would we want 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.
Presets that define 0 parameters are valid. They're used in more trivial templates, usually docker based, where there really isn't much to change but one still wants prebuilds. In this case one would define a preset with no parameters that desiresn prebuilt workspaces. This form of the query allows a workspace in such a template to still match a preset and therefore claim a prebuilt workspace.
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 is useful for simpler parameters where one wants prebuilds without specifying any parameters. You could define a preset that sets no parameters. If such a preset exists, it would be selected automatically by this.
| iferr!=nil { | ||
| returnBuildError{http.StatusInternalServerError,"find matching preset",err} | ||
| } | ||
| b.templateVersionPresetID=presetID |
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 code path could suggest to the reader that there will always be a match, a comment mentioning it will be nil if none is found may be beneficial to avoid wrong assumptions (since we're not returning error from find).
f9a6adc intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#18356.
This change finds and selects a matching preset if one was not chosen during workspace creation. This solidifies the relationship between presets and parameters.
When a workspace is created without in explicitly chosen preset, it will now still be eligible to claim a prebuilt workspace if one is available.