Skip to content

Conversation

@magaldima
Copy link
Contributor

@magaldima magaldima commented Jan 24, 2025

Resolves #725

Copy link
Contributor

@bgentry bgentry 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 opening this! Could you please add yourself to the CLA since this is your first contribution?

Could you also add test coverage for the new config option? For a simple bool field like this, just adding it to the tests Test_NewClient_Defaults and Test_NewClient_Overrides here should be sufficient. This is essentially just making sure it's propagated through to the client's inner config when the copy is done.

Finally, we'll want some proper test coverage for this as well. We have cases for each insert method named AllowsUnknownJobKindWithoutWorkers (6 in total, we should really dedupe the logic now that the insert path and behavior are mostly shared). I think we'll want to add a case below each of those for AllowsUnknownJobKindWith SkipUnknownJobCheck to ensure that an unknown job can be inserted even if workers are registered if that config is true.

client.go Outdated
// the client's insertion behavior mimic that of an insert-only client.
//
// Defaults to false.
SkipUnknownJobCheck bool
Copy link
Contributor

Choose a reason for hiding this comment

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

We tend to keep struct fields sorted alphabetically unless there's a compelling reason not to. Could you put this down above TestOnly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated! thanks for the feedback!

@magaldima
Copy link
Contributor Author

magaldima commented Jan 24, 2025

Signed CLA here: riverqueue/rivercla#8. Will work on the updates & tests and get that updated soon.

@magaldima magaldima requested a review from bgentry January 24, 2025 18:25
@brandur
Copy link
Contributor

brandur commented Jan 25, 2025

Great work. Thank you!

Would you mind adding this line in CHANGELOG.md under the unreleased "added" section?

- Add `SkipUnknownJobCheck` client config option to skip job arg worker validation. [PR #731](https://github.com/riverqueue/river/pull/731).

Copy link
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking the lead on this feature PR! Hoping to get a release out today with this and other changes. 🚀

@bgentry bgentry merged commit fdbc2dd into riverqueue:master Jan 25, 2025
10 checks passed
@bgentry bgentry mentioned this pull request Jan 28, 2025
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.

3 participants