-
-
Notifications
You must be signed in to change notification settings - Fork 9
feat(job): add options to override tasks' defaults #105
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
base: main
Are you sure you want to change the base?
Conversation
|
At first glance, seems like a reasonable approach. |
|
Thanks for the fast reply :) So, to keep this going I will:
Should it follow the same layout as Also, check is failing for something not touched in this PR... |
|
It's probably a little easier to keep it in its own module, yes. |
|
This isn't something that has to be solved in this PR, but I do want to note that there's a lot of overlap with this new type and the task trait itself; maybe in the future they could share an options struct. |
4558029 to
918a60b
Compare
|
I see what you mean but I think that the To unify the interfaces I think we would need to either:
Keeping both might bring some confusion as to which implementation (static or the callback) would have priority. Anyway, I think I've added a test and the builder so this may be ready for another round of review. |
|
This should be rebased on |
918a60b to
6420642
Compare
|
@maxcountryman updated! |
6420642 to
ee57974
Compare
|
re-formatted with nightly! |
StepOptions allow us to override defaults for timeout, heartbeat and so on. It is necessary when you want to use Context and different timeouts for example.
ee57974 to
8ef0ba7
Compare
|
Okay. Sorry for the noise... should have run all the checks before. I believe NOW everything is passing. |
|
I'm going to merge this but before cutting a new release I'd like to iterate a bit on the implementation to address duplication between Task and Job. I have an idea but the end result might change the API introduced here somewhat. |
|
Awesome! I currently point to a private fork and there is no problem about not releasing a new version here. Thanks a lot for this! |
Hi @maxcountryman !
This is a first pass into adding options to each individual step in a job workflow. I needed this to override task timeouts (in my case I had a long job that was waiting for a task completion).
I know documentation is not yet good and there aren't any tests, but I just wanted to know if this is ok implementation wise. Before merging we will probably need to create a builder to keep the same style and add tests and better documentation.
I thought about adding an option like
transaction_in_context: boolbut I think this is better suited in another PR because it will be a bit bigger and probably more breaking than this one.Thanks once again for this great library.