chore: bunify postgres_jobs.go #8858
Conversation
✅ Deploy Preview for determined-ui canceled.
|
proto/pkg/apiv1/api.pb.go
Outdated
There was a problem hiding this comment.
I actually don't know why this file changed. I did not intentionally run any proto commands to update this file. Although I did run make commands like build/fmt/check.
There was a problem hiding this comment.
I think this is the reason the change is not rebasable; I suspect I need to overwrite the file with whatever is currently in master?
There was a problem hiding this comment.
I've never run into this issue with a rebase, though maybe you did something slightly differently. Sounds plausible this is some artifact of git process though.
| @@ -19,7 +19,7 @@ import ( | |||
| "github.com/determined-ai/determined/master/internal/api" | |||
| "github.com/determined-ai/determined/master/internal/authz" | |||
| "github.com/determined-ai/determined/master/internal/checkpoints" | |||
| "github.com/determined-ai/determined/master/internal/db" | |||
| internaldb "github.com/determined-ai/determined/master/internal/db" | |||
There was a problem hiding this comment.
named import as internaldb to differentiate between db the type and db the package import; I think this is more clear (especially since db is sometimes used in multiple contexts in the same file) but can undo it if it's against style guidelines
There was a problem hiding this comment.
I've done the db conflicts with db dance enough that I'm fine with it.
kkunapuli
requested review from
sandrews-hpe,
carolinaecalderon and
stoksc
and removed request for
carolinaecalderon and
sandrews-hpe
| @@ -19,7 +19,7 @@ import ( | |||
| "github.com/determined-ai/determined/master/internal/api" | |||
| "github.com/determined-ai/determined/master/internal/authz" | |||
| "github.com/determined-ai/determined/master/internal/checkpoints" | |||
| "github.com/determined-ai/determined/master/internal/db" | |||
| internaldb "github.com/determined-ai/determined/master/internal/db" | |||
There was a problem hiding this comment.
I've done the db conflicts with db dance enough that I'm fine with it.
master/internal/db/postgres_jobs.go
Outdated
| `, &j, jID); err != nil { | ||
| return nil, errors.Wrap(err, "querying job") | ||
| func JobByID(ctx context.Context, jobID model.JobID) (*model.Job, error) { | ||
| j := model.Job{} |
There was a problem hiding this comment.
style nit: https://github.com/uber-go/guide/blob/master/style.md#use-var-for-zero-value-structs
note, all style is up for discussion so if you disagree don't hesitate
master/internal/db/postgres_jobs.go
Outdated
| j := model.Job{} | ||
| err := Bun().NewSelect().Model(&j). | ||
| Where("job_id = ?", jobID). | ||
| Scan(context.TODO()) |
There was a problem hiding this comment.
use the ctx that is passed.
| require.NoError(t, err, "failed to add job") | ||
|
|
||
| // Retrieve it back and make sure the mapping is exhaustive. | ||
| jOut, err := db.JobByID(jID) | ||
| jOut, err := JobByID(context.TODO(), jID) |
There was a problem hiding this comment.
it is fine to use context.Background() for tests. so the TODOs are all just real TODOs.
proto/pkg/apiv1/api.pb.go
Outdated
There was a problem hiding this comment.
I've never run into this issue with a rebase, though maybe you did something slightly differently. Sounds plausible this is some artifact of git process though.
stoksc
force-pushed
the
kunapuli/bunify-postres-jobs
branch
from
e31ab2c
to
644bedb
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #8858 +/- ##
==========================================
- Coverage 47.47% 47.47% -0.01%
==========================================
Files 1066 1066
Lines 170207 170209 +2
Branches 2243 2241 -2
==========================================
Hits 80812 80812
- Misses 89237 89239 +2
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more.
|
* chore: bunify postgres_jobs.go * attempt to restore proto changes * trying again to restore file * PR feedback: use var for empty struct and context.Background in tests * retry ci tests
Description
DET-10134 Convert all SQL commands in postgres_jobs.go to Bun SQL commands.
Test Plan
Using devcluster locally, created an experiment. Exec'd into postgres container to confirm a new job exists.
Commentary (optional)
Moving to bun means that we are also slowly simplifying (deprecating?) the db interface, allowing code to move out of the db package and be organized in a more modular way.
I also named /master/internal/db import as
internaldbto differentiate between db as a type and db as a package - especially since it was sometimes used in both contexts in the same file.Checklist
docs/release-notes/.See Release Note for details.
Ticket
DET-10134