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 support for trigger listeners #595

Merged
merged 1 commit into from Dec 5, 2018

Conversation

Projects
None yet
2 participants
@cdupuis
Copy link
Contributor

cdupuis commented Dec 5, 2018

Allows to register listeners that run based on configured intervals in milliseconds or cron patterns.

@cdupuis cdupuis requested review from johnsonr and ddgenome Dec 5, 2018

@cdupuis cdupuis force-pushed the triggered-listener branch from 10f3c2b to ea34ab8 Dec 5, 2018

@cdupuis cdupuis force-pushed the triggered-listener branch from ea34ab8 to 48647d9 Dec 5, 2018

@cdupuis cdupuis merged commit 3b0bd50 into master Dec 5, 2018

2 checks passed

license/cla Contributor License Agreement is signed.
Details
sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals: all succeeded
Details

@cdupuis cdupuis deleted the triggered-listener branch Dec 5, 2018

atomist-bot added a commit that referenced this pull request Dec 5, 2018

Changelog: #595 to added
[atomist:generated]
@ddgenome
Copy link
Member

ddgenome left a comment

A couple questions.

} as any);
cron.start();
}
if (t.trigger && t.trigger.interval) {

This comment has been minimized.

@ddgenome

ddgenome Dec 5, 2018

Member

Could you have both cron and trigger? Docs indicate otherwise but this would allow it.

This comment has been minimized.

@cdupuis

cdupuis Dec 5, 2018

Author Contributor

Yes, you can have both. I'll update the comment to reflect that.

let count = 0;
const job: TriggeredListenerRegistration = {
trigger: {
cron: "*/2 * * * * *",

This comment has been minimized.

@ddgenome

ddgenome Dec 5, 2018

Member

* * * * * * might speed things up

.then(() => {
const d = new Deferred<any>();
setTimeout(() => {
assert(count >= 2);

This comment has been minimized.

@ddgenome

ddgenome Dec 5, 2018

Member

Is this is currently only firing every even minute, how is it incrementing at least twice within ten seconds?

This comment has been minimized.

@cdupuis

This comment has been minimized.

@cdupuis

cdupuis Dec 5, 2018

Author Contributor

cc @ddgenome did you see this? Maybe it is not the best library if it isn't following crontab expressions. Although that was the same with Quartz for Java if I remember correctly.

This comment has been minimized.

@ddgenome
let count = 0;
const job: TriggeredListenerRegistration = {
trigger: {
interval: 1000,

This comment has been minimized.

@ddgenome

ddgenome Dec 5, 2018

Member

Could reduce to 100, shorten Deferred timeout, and remove increasing test timeout.

This comment has been minimized.

@cdupuis

cdupuis Dec 5, 2018

Author Contributor

Will do.

export interface TriggeredListenerRegistration {

/**
* Trigger can either be a cron expression as string or a number in milliseconds

This comment has been minimized.

@ddgenome

ddgenome Dec 5, 2018

Member

"or" but code does an "and"

This comment has been minimized.

@cdupuis

cdupuis Dec 5, 2018

Author Contributor

Fixed

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Dec 5, 2018

I guess I was too late.

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

cdupuis commented Dec 5, 2018

Yeah sorry @ddgenome for the merge. I had other pending changes on some of the same files and I didn't want to create too many merge conflicts by maintaining to separate branches. I can still address all comments if you think it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment