Skip to content

fix: Order database queries for templates #3249

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 27, 2022
Merged

fix: Order database queries for templates #3249

merged 4 commits into from
Jul 27, 2022

Conversation

mafredri
Copy link
Member

Fixes a race in a test where the order of templates varies.

    ptytest.go:50: cmd: stdout: "NAME              LAST UPDATED   USED BY       "
372
    ptytest.go:50: cmd: stdout: "pedantic-galois4  July 27, 2022  0 developers  "
373
    ptytest.go:50: cmd: stdout: "strange-booth8    July 27, 2022  0 developers  "
374
    templatelist_test.go:41: 2022-07-27 13:39:37.097225345 +0000 UTC m=+89.235838173: matched "strange-booth8" = "NAME              LAST UPDATED   USED BY       \r\npedantic-galois4  July 27, 2022  0 developers  \r\nstrange-booth8"
375
    t.go:81: 2022-07-27 13:39:39.140 [DEBUG]	<github.com/coder/coder/coderd/provisionerdaemons.go:98>	(*API).ListenProvisionerDaemon.func2	drpc server error ...
376
        "error": stream closed
377
                 	storj.io/drpc/drpcstream.(*Stream).sendPacket:268
378
                 	storj.io/drpc/drpcstream.(*Stream).CloseSend:501
379
                 	storj.io/drpc/drpcserver.(*Server).handleRPC:126
380
                 	storj.io/drpc/drpcserver.(*Server).ServeOne:66
381
                 	storj.io/drpc/drpcserver.(*Server).Serve.func2:112
382
                 	storj.io/drpc/drpcctx.(*Tracker).track:52
383
    t.go:81: 2022-07-27 13:39:41.144 [DEBUG]	<github.com/coder/coder/coderd/provisionerdaemons.go:98>	(*API).ListenProvisionerDaemon.func2	drpc server error ...
384
        "error": stream closed
385
                 	storj.io/drpc/drpcstream.(*Stream).sendPacket:268
386
                 	storj.io/drpc/drpcstream.(*Stream).CloseSend:501
387
                 	storj.io/drpc/drpcserver.(*Server).handleRPC:126
388
                 	storj.io/drpc/drpcserver.(*Server).ServeOne:66
389
                 	storj.io/drpc/drpcserver.(*Server).Serve.func2:112
390
                 	storj.io/drpc/drpcctx.(*Tracker).track:52
391
    templatelist_test.go:42: 2022-07-27 13:39:47.110267312 +0000 UTC m=+99.248880040: match exceeded deadline: wanted "pedantic-galois4"; got "    July 27, 2022  0 developers  \r\n"
392
    t.go:81: 2022-07-27 13:39:47.133 [DEBUG]	(provisionerd)	<github.com/coder/coder/provisionerd/provisionerd.go:369>	(*Server).closeWithError	closing server with error	{"error": null}
393
    --- FAIL: TestTemplateList/ListTemplates (13.96s)

@mafredri
fix: Order database queries for templates
1a64acd 
Fixes a race in a test where the order of templates varies.
@mafredri mafredri self-assigned this Jul 27, 2022
@mafredri mafredri requested a review from a team July 27, 2022 14:06
mafredri
mafredri commented Jul 27, 2022
View reviewed changes
coderd/database/queries/templates.sql
@@ -49,7 +50,9 @@ LIMIT
1;

-- name: GetTemplates :many
SELECT * FROM templates;
SELECT * FROM templates
ORDER BY (created_at, id) ASC
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't relevant for the test failure, but a bonus fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the fake db need this too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing not, since they'll be appended to a slice. But I can add it there as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, we should explicitly sort to match the sql imo. I do this with a copied slice somewhere else in the fake db where ordering matters.

Copy link
Member Author

@mafredri mafredri Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c0498a9, nevermind... f8df8ef, nevermind... 97e0667 (sort functions :smh:).

AbhineetJain
AbhineetJain approved these changes Jul 27, 2022
View reviewed changes
Copy link
Contributor

@AbhineetJain AbhineetJain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this!

@mafredri mafredri enabled auto-merge (squash) July 27, 2022 15:01
@mafredri mafredri merged commit cef622d into main Jul 27, 2022
@mafredri mafredri deleted the mafredri/fix-template-list-order branch July 27, 2022 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emyrk Emyrk Emyrk left review comments

@johnstcn johnstcn johnstcn approved these changes

@AbhineetJain AbhineetJain AbhineetJain approved these changes

Assignees

@mafredri mafredri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mafredri @johnstcn @Emyrk @AbhineetJain