-
Notifications
You must be signed in to change notification settings - Fork 132
feat(config): add SkipUnknownJobCheck client config option to skip job arg worker validation #731
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
feat(config): add SkipUnknownJobCheck client config option to skip job arg worker validation #731
Conversation
…b arg worker validation
bgentry
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
|
Signed CLA here: riverqueue/rivercla#8. Will work on the updates & tests and get that updated soon. |
|
Great work. Thank you! Would you mind adding this line in - Add `SkipUnknownJobCheck` client config option to skip job arg worker validation. [PR #731](https://github.com/riverqueue/river/pull/731). |
bgentry
left a comment
There was a problem hiding this 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. 🚀
Resolves #725