-
Notifications
You must be signed in to change notification settings - Fork 92
feat: add jobs #688
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?
feat: add jobs #688
Conversation
@atrauzzi This runs a linter (for now) as part of the build pipeline and failures will stop the build. Would you mind fixing the lint errors locally and pushing a commit with those changes as well so we can run the checks? |
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.
Excellent start - still a bit to go, but it's a good foundation.
src/interfaces/Server/IServerJobs.ts
Outdated
import { TypeDaprJobsCallback } from "../../types/DaprJobsCallback.type"; | ||
|
||
export default interface IServerJobs { | ||
listen(jobName: string, callback: TypeDaprJobsCallback): void; |
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.
Same here - I don't know that the server will ever pass along a job invocation without a payload (e.g. though of course it might be empty).
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.
See above 🙂
src/types/jobs/JobSchedule.type.ts
Outdated
|
||
type EveryPeriod = `@every ${string}`; | ||
|
||
// note: This can get crazy, more than TS can really handle. |
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.
You might consider taking a look at how I built this out in the .NET SDK to accommodate each variation.
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.
Most importantly - there are different sets of arguments that need to be provided based on this schedule and the other arguments used.
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.
Mind elaborating on the different sets of arguments? I expanded things a fair bit in a recent push, maybe you wanna look at that and if you want, we can discuss here or in discord 🙂
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.
Coming along - note about the regexps though
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.
More thoughts
src/interfaces/Client/IClientJobs.ts
Outdated
@@ -11,17 +11,17 @@ See the License for the specific language governing permissions and | |||
limitations under the License. | |||
*/ | |||
|
|||
import { JobSchedule } from "../../types/jobs/JobSchedule.type"; | |||
import { Schedule } from "../../types/jobs/JobSchedule.type"; |
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.
Especially if the file only contains a single type, the type name should match the file name.
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.
Yep, agreed.
listen(jobName: string, callback: TypeDaprJobsCallback): void; | ||
|
||
listen<DataType>(jobName: string, callback: TypeDaprJobsCallback<DataType>): void; | ||
listen(jobName: string, callback: (data: any) => unknown): void; |
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.
Why does this return an unknown
? While the callback needn't necessarily return anything, the SDK does need to know that it was handled without error so it can send back a 200 OK.
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.
That's the callback that users provide. I mark it as unknown so that it can be optionally sync or async.
Should we be imposing some kind of contract for the implementation to return, or just assume handling was successful if no exception is thrown?
- Addresses - dapr#688 (comment) - dapr#688 (comment) Signed-off-by: Alexander Trauzzi <[email protected]>
export type SecondOrMinute = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 | 30 | 31 | 32 | 33 | 34 | 35 | 36 | 37 | 38 | 39 | 40 | 41 | 42 | 43 | 44 | 45 | 46 | 47 | 48 | 49 | 50 | 51 | 52 | 53 | 54 | 55 | 56 | 57 | 58 | 59; | ||
export type Hour = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23; | ||
export type DayOfMonth = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | 14 | 15 | 16 | 17 | 18 | 19 | 20 | 21 | 22 | 23 | 24 | 25 | 26 | 27 | 28 | 29 | 30 | 31; | ||
export type DayOfWeekNumber = 0 | 1 | 2 | 3 | 4 | 5 | 6; | ||
export type MonthNumber = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12; |
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.
This is a bit of a NIT, but theres a different way of adding type for these situations, which ive used before.
You would have first a type that would help you build such types:
type BuildNumberRange<T extends number, U extends number, R extends number[] = []> = R['length'] extends U
? R[number]
: BuildNumberRange<T, U, [...R, R['length'] & number]>;
And then you can have your types like this:
export type SecondOrMinute = BuildNumberRange<0, 59>
export type Hour = BuildNumberRange<0, 23>
export type DayOfMonth = BuildNumberRange<1, 31>
export type DayOfWeekNumber = BuildNumberRange<0, 6>
export type MonthNumber = BuildNumberRange<1, 12>
Just an idea. I realise the BuildNumberRange
type is quite hard to reason with for new comers, but its the type of "set it and forget it".
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.
That's pretty slick.
|
What's this in reference to? |
@WhitWaldo This is the failure from the test run of the previous commit. However, I don't think it sheds much light. I think the interesting failure is in the scheduler's (Testcontainer) log:
I suspect this |
Alright, so a small bit of progress on connectivity between the dapr daemon and the scheduler, but sadly not quite there with things yet. I'm now running into a timeout scenario where it seems like any attempt to use the client. dapr-js-sdk testcontainers jest timeout.txt The code seems to hang when I try to create the job. @WhitWaldo, maybe you can spot something? |
test/e2e/jobs/jobs.test.ts
Outdated
.withTmpFs({ | ||
"/data": "rw", | ||
}) | ||
// note: Because dapr containers don't have `sh` or `bash` inside, this is kind of the best health check. |
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.
Two comments:
- Wait.forHttp(path, port) works for me, and is what Dapr's Testcontainer for Java does:
- The timeout should be much longer, e.g., 120_000
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.
I suggest something like the following for the wait strategy
.withWaitStrategy(Wait.forHttp("/v1.0/healthz/outbound", DAPRD_HTTP_PORT)
.forStatusCodeMatching((statusCode) => statusCode >= 200 && statusCode <= 399))
.withStartupTimeout(120_000)
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.
This has proven to be the trickiest, but also avoidable if I stick with my prior approach, waiting for console output. For some reason that endpoint never seems to ready up.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #688 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 6 6
Branches 1 1
=========================================
Hits 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
(...make Jack a dull boy) Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
- Addresses - dapr#688 (comment) - dapr#688 (comment) Signed-off-by: Alexander Trauzzi <[email protected]>
* Removing prettifier from the build pipeline as it's an unnecessary step during a build operation. * Version number updated in package-lock.json Signed-off-by: Whit Waldo <[email protected]> Signed-off-by: Alexander Trauzzi <[email protected]>
Removed references to Workflow components and replaced with "dapr" for the name in the HTTP client. Signed-off-by: Whit Waldo <[email protected]> Signed-off-by: Alexander Trauzzi <[email protected]>
Removes stale bot + adds me to repo owners for bot actions Signed-off-by: Whit Waldo <[email protected]> Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Joe Bowbeer <[email protected]> Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]> # Conflicts: # package-lock.json
Signed-off-by: Alexander Trauzzi <[email protected]>
Signed-off-by: Alexander Trauzzi <[email protected]>
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.
Just want to make sure we've got strong test coverage - reviewed on GitHub, so I can't run a check myself, but if it looks good to you, I'm satisifed.
Also, please remove the gRPC implementations of the Jobs bits. The way I see it, the SDK need only implement one or the other and it's up to the runtime to ensure that there's parity in the APIs. We have no need to expose both and have the developer pick one as that would just be confusing. Rather, you've already got HTTP in place, so let's delete the gRPC side.
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.
Frankly, I think the SDK should just pick one or the other - if we've got this implemented via HTTP, I don't know we need to have a stub for gRPC (that itself doesn't work) just because as that can lead to developer confusion.
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.
As before - if this is implemented using HTTP, no need to add a gRPC implementation just because.
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.
Could you add a unit test that proves this out?
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.
I think the shape of this has changed in the latest protos - I believe there's an override property now.
} | ||
|
||
export class CronExpression { | ||
public static readonly SecondsAndMinutesRegexText = /(?:\*(?:\/[0-5]?\d)?|(?:[0-5]?\d)(?:-(?:[0-5]?\d))?(?:,(?:[0-5]?\d)(?:-(?:[0-5]?\d))?)*)/; |
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.
Please add unit tests to prove each of these regexes
.join(","); | ||
} | ||
|
||
private prepareDayOfWeekValueList(dayOfWeekValues: DayOfWeek[]) { |
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.
As this doesn't reference DayOfWeekValues, are you sure this will sort properly?
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.
I'd just generally like to see much better test coverage to prove there are no typos in each of the helper extension implementations.
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.
When putting together each of the regex variations, just do a quick pass to make sure any seemingly edge cases are covered.
Description
Added initial support for jobs.
Issue reference
Closes: #667
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: