Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JobGet + JobGetTx methods to Client, use within tests #186

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

bgentry
Copy link
Contributor

@bgentry bgentry commented Feb 1, 2024

This adds some basic APIs for fetching a single job from the database. I updated all the tests in client_test.go to use it, except for some lower level ones that were using dbsqlc types and didn't seem worth converting.

@bgentry bgentry requested a review from brandur February 1, 2024 14:52
@bgentry
Copy link
Contributor Author

bgentry commented Feb 1, 2024

Lol, messed up my commits, fixing.

Copy link
Contributor

@brandur brandur left a comment

Choose a reason for hiding this comment

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

Looks great!

Should we cut a new release after merging this one? We may not ship anything else interesting for another week or two.


client, _ := setup(t)

job, err := client.JobGet(ctx, 999999)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hah, very unlikely, but small chance this fails locally because our test DB sequences don't get reset between runs. Probably will never happen, but I guess zero might be a slighter safer alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should only be an issue if a row actually exists with this ID, right? And we either truncate after each test or use a txn that gets rolled back.

@bgentry
Copy link
Contributor Author

bgentry commented Feb 2, 2024

Should we cut a new release after merging this one? We may not ship anything else interesting for another week or two.

I don't think so quite yet. I will have another new API up soon (something like JobRetryNow to immediately schedule any job that's not currently running) so maybe after that we can cut a new one. Plus I'd like to test both of these further in my UI repo just to be sure.

@bgentry bgentry merged commit 4ea5395 into master Feb 2, 2024
7 checks passed
@bgentry bgentry deleted the bg-job-get branch February 2, 2024 03:25
brandur added a commit that referenced this pull request Feb 11, 2024
It's been two weeks since our last release and we have a couple nice
features in `master` like a job retrieval API (#186) and a job retry API
(#190). Let's keep the releases rolling. Here, prep `v0.0.19`.
@brandur brandur mentioned this pull request Feb 11, 2024
brandur added a commit that referenced this pull request Feb 11, 2024
It's been two weeks since our last release and we have a couple nice
features in `master` like a job retrieval API (#186) and a job retry API
(#190). Let's keep the releases rolling. Here, prep `v0.0.19`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants