Skip to content

Commit 85b1ef4

Browse files
SasSwartSasSwart
committed
review notes
1 parent ff75f5e commit 85b1ef4

File tree

14 files changed

+70
-65
lines changed

14 files changed

+70
-65
lines changed

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2560,7 +2560,7 @@ func (s *MethodTestSuite) TestSystemFunctions() {
25602560
j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{
25612561
StartedAt: sql.NullTime{Valid: false},
25622562
})
2563-
check.Args(database.AcquireProvisionerJobParams{OrganizationID: j.OrganizationID, Types: []database.ProvisionerType{j.Provisioner}, Tags: must(json.Marshal(j.Tags))}).
2563+
check.Args(database.AcquireProvisionerJobParams{OrganizationID: j.OrganizationID, Types: []database.ProvisionerType{j.Provisioner}, ProvisionerTags: must(json.Marshal(j.Tags))}).
25642564
Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/ )
25652565
}))
25662566
s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) {

‎coderd/database/dbfake/dbfake.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,8 +194,8 @@ func (b WorkspaceBuildBuilder) Do() WorkspaceResponse {
194194
UUID: uuid.New(),
195195
Valid: true,
196196
},
197-
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
198-
Tags: []byte(`{"scope": "organization"}`),
197+
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
198+
ProvisionerTags: []byte(`{"scope": "organization"}`),
199199
})
200200
require.NoError(b.t, err, "acquire starting job")
201201
if j.ID == job.ID {

‎coderd/database/dbgen/dbgen.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -531,11 +531,11 @@ func ProvisionerJob(t testing.TB, db database.Store, ps pubsub.Pubsub, orig data
531531
}
532532
if !orig.StartedAt.Time.IsZero() {
533533
job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{
534-
StartedAt: orig.StartedAt,
535-
OrganizationID: job.OrganizationID,
536-
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
537-
Tags: must(json.Marshal(orig.Tags)),
538-
WorkerID: uuid.NullUUID{},
534+
StartedAt: orig.StartedAt,
535+
OrganizationID: job.OrganizationID,
536+
Types: []database.ProvisionerType{database.ProvisionerTypeEcho},
537+
ProvisionerTags: must(json.Marshal(orig.Tags)),
538+
WorkerID: uuid.NullUUID{},
539539
})
540540
require.NoError(t, err)
541541
// There is no easy way to make sure we acquire the correct job.

‎coderd/database/dbmem/dbmem.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1194,8 +1194,8 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu
11941194
continue
11951195
}
11961196
tags := map[string]string{}
1197-
if arg.Tags != nil {
1198-
err := json.Unmarshal(arg.Tags, &tags)
1197+
if arg.ProvisionerTags != nil {
1198+
err := json.Unmarshal(arg.ProvisionerTags, &tags)
11991199
if err != nil {
12001200
return provisionerJob, xerrors.Errorf("unmarshal: %w", err)
12011201
}
@@ -3634,11 +3634,9 @@ func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg
36343634
if daemon.OrganizationID != arg.OrganizationID {
36353635
continue
36363636
}
3637-
if arg.Tags != nil {
3638-
for k, v := range arg.Tags {
3639-
if t, found := daemon.Tags[k]; !found || t != v {
3640-
continue
3641-
}
3637+
for k, v := range arg.WantTags {
3638+
if t, found := daemon.Tags[k]; !found || t != v {
3639+
continue
36423640
}
36433641
}
36443642
daemon.Tags = maps.Clone(daemon.Tags)

‎coderd/database/dump.sql

Lines changed: 11 additions & 8 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000274_check_tags.down.sql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
1-
DROP FUNCTION IF EXISTS tags_compatible(tags, tags);
1+
DROP FUNCTION IF EXISTS tagset_contains(tags, tags);
22

33
DROP DOMAIN IF EXISTS tags;

‎coderd/database/migrations/000274_check_tags.up.sql

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
1-
-- We need this as a type alias to ensure that we can override it in sqlc.
2-
-- We need to override it in sqlc so that it can be recognised as a StringMap.
3-
-- Without this zero values for other inferred types cause json syntax errors.
4-
CREATE DOMAIN tags AS jsonb;
1+
CREATE DOMAIN tagset AS jsonb;
52

6-
CREATE OR REPLACE FUNCTION tags_compatible(subset_tags tags, superset_tags tags)
3+
COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.';
4+
5+
CREATE OR REPLACE FUNCTION tagset_contains(superset tagset, subset tagset)
76
RETURNS boolean as $$
87
BEGIN
9-
RETURN CASE
10-
-- Special case for untagged provisioners
11-
WHEN subset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb
12-
THEN subset_tags = superset_tags
13-
ELSE subset_tags :: jsonb <@ superset_tags :: jsonb
14-
END;
8+
RETURN
9+
-- Special case for untagged provisioners, where only an exact match should count
10+
(subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset)
11+
-- General case
12+
OR subset <@ superset;
1513
END;
1614
$$ LANGUAGE plpgsql;
15+
16+
COMMENT ON FUNCTION tagset_contains(tagset, tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.';

‎coderd/database/querier_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1020,7 +1020,7 @@ func TestQueuePosition(t *testing.T) {
10201020
UUID: uuid.New(),
10211021
Valid: true,
10221022
},
1023-
Tags: json.RawMessage("{}"),
1023+
ProvisionerTags: json.RawMessage("{}"),
10241024
})
10251025
require.NoError(t, err)
10261026
require.Equal(t, jobs[0].ID, job.ID)

‎coderd/database/queries.sql.go

Lines changed: 17 additions & 15 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/provisionerdaemons.sql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ WHERE
1414
organization_id = @organization_id :: uuid
1515
AND
1616
-- adding support for searching by tags:
17-
(@tags :: tags = 'null' :: tags OR tags_compatible(@tags::tags, provisioner_daemons.tags::tags));
17+
(@want_tags :: tagset = 'null' :: tagset OR tagset_contains(provisioner_daemons.tags::tagset, @want_tags::tagset));
1818

1919
-- name: DeleteOldProvisionerDaemons :exec
2020
-- Delete provisioner daemons that have been created at least a week ago

0 commit comments

Comments
 (0)