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 `Queue` goal to queue goal sets #597

Merged
merged 1 commit into from Dec 7, 2018

Conversation

Projects
None yet
2 participants
@cdupuis
Copy link
Contributor

cdupuis commented Dec 6, 2018

The Queue maintains a queue of goals sets of configurable size. Once a goal set is terminated, the queue goal will trigger the next waiting goal sets, etc.

I also fixed a couple of bugs in the waitRules and missing state on SdmGoalSet.

@cdupuis cdupuis requested a review from ddgenome Dec 6, 2018

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

cdupuis commented Dec 6, 2018

@ddgenome I'll add a test for the Queue goal later but wanted to get this up for review.

@ddgenome
Copy link
Member

ddgenome left a comment

LGTM

return SdmGoalState.planned;
} else if (!goals.some(g => g.state !== SdmGoalState.success)) {
return SdmGoalState.success;
}

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

The last check is negated. Is there any way that none of these conditionals match?

This comment has been minimized.

@cdupuis

cdupuis Dec 6, 2018

Author Contributor

Right now there isn't. I added checks for all possible statuses before checking for success. In fact it might just be an else. WDYT?

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

I think it would be clearer, but not required for this PR.

This comment has been minimized.

@cdupuis

cdupuis Dec 6, 2018

Author Contributor

Ok, I’ll change that.

setTimeout(() => resolve(), timeoutMillis).unref();
function wait(timeoutMillis: number, unref: boolean): Promise<any> {
return new Promise<any>(resolve => {
const timer = setTimeout(() => resolve(), timeoutMillis);

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

This could just be setTimeout(resolve, timeoutMillis).

This comment has been minimized.

This comment has been minimized.

@cdupuis

cdupuis Dec 6, 2018

Author Contributor

Oh, yeah. That's a nice one.

@cdupuis cdupuis force-pushed the goalset-state branch 2 times, most recently from e0246d9 to 86e70ba Dec 6, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

Did you remove some of the code? In particular, the stuff that has changed in the back and forth?

function wait(timeoutMillis: number): Promise<void> {
return new Promise<void>(resolve => {
setTimeout(() => resolve(), timeoutMillis).unref();
function wait(timeoutMillis: number, unref: boolean): Promise<any> {

This comment has been minimized.

@ddgenome

ddgenome Dec 6, 2018

Member

Isn't it still returning Promise<void>?

This comment has been minimized.

@cdupuis

cdupuis Dec 6, 2018

Author Contributor

Ah, yes, you’re right. Not sure what i thought there.

@cdupuis

This comment has been minimized.

Copy link
Contributor Author

cdupuis commented Dec 6, 2018

What code are you missing @ddgenome?

@ddgenome

This comment has been minimized.

Copy link
Member

ddgenome commented Dec 6, 2018

Nothing, I was conflating this PR and atomist/automation-client#410

@cdupuis cdupuis force-pushed the goalset-state branch from 86e70ba to e1a46a4 Dec 7, 2018

@cdupuis cdupuis force-pushed the goalset-state branch from e1a46a4 to 20daa1f Dec 7, 2018

@ddgenome
Copy link
Member

ddgenome left a comment

LGTM

@ddgenome ddgenome merged commit 602bec1 into master Dec 7, 2018

1 of 2 checks passed

sdm/atomist/atomist-sdm Atomist Software Delivery Machine goals in progress
Details
license/cla Contributor License Agreement is signed.
Details

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

Changelog: #597 to added
[atomist:generated]

@ddgenome ddgenome deleted the goalset-state branch Dec 7, 2018

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