- Notifications
You must be signed in to change notification settings - Fork928
feat(coderd/database): support exact tags match in AcquireProvisionerJob query#12244
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
9b77b42 to3796e17Compare…ecifying exact tag match behaviour
3796e17 to6c18525CompareThere 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 a nice and concise PR! Left 2 nit-picks.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| AND nested.tags <@ @tags :: jsonb | ||
| -- Ensure the caller satisfies all job tags if requested, | ||
| AND CASE | ||
| WHEN @exact_tag_match :: boolean THEN nested.tags = @tags :: jsonb |
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.
Are both guaranteed to be sorted arrays? Might be a good idea to allow unsorted equality.
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 they'remap[string]string so we want to completely ignore order in comparisons.
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.
Ah right, then ajsonb tojsonb comparison should be fine 👍🏻
Putting the scope of the change here has the unfortunate side-effect of enforcing a global tag policy. I'm going to make additional modifications to modify the scope of acquirer.AcquireJob and have the parameter be passed in from provisionerdserver. This should allow individual behaviour per provisioner daemon as we start a provisionerdserver for each provisioner daemon. |
Alternative solution:#12269 I'm leaning in favour of this approach as it doesn't require another configuration knob. |
Uh oh!
There was an error while loading.Please reload this page.
Part of#6442
Updates AcquireProvisionerJob query with parameter ExactTagMatch which toggles between existing behaviour (subset matches) or new behaviour (all match).
Adds unit tests for new behaviour.
A separate PR will wire this up to a configuration knob in coderd and provisionerd.