feat(3181): Skip execution of a virtual job when an event is started from that job #3252

sagar1312 · 2024-12-12T17:51:22Z

Context

Per changes made in screwdriver-cd/models#635, virtual job builds at the beginning in the workflow for an event, may not be queued for execution. Status of such build will be set to CREATED.

Objective

Identify such build immediately after creating the event to mark its status as 'SUCCESS' and trigger its downstream jobs.

References

#3181

License

I confirm that this contribution is made under a BSD license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

…from that job

coveralls · 2024-12-12T22:52:03Z

coverage: 95.306% (+0.1%) from 95.173%
when pulling 409534a on sagar1312-feat-3181-skip-virtual-job-as-event-origin
into f71ebdf on master.

VonnyJap · 2024-12-12T22:54:32Z

plugins/builds/helper/updateBuild.js

+        });
+    } else if (desiredStatus === 'QUEUED' && currentStatus !== 'QUEUED') {
+        throw boom.badRequest(`Cannot update builds to ${desiredStatus}`);
+    } else if (desiredStatus === 'BLOCKED' && currentStatus === 'BLOCKED') {


making sure that currentStatus === 'BLOCKED' is what needed here and not currentStatus !== 'BLOCKED'.

This is an existing code. Just moved it to a common module. Avoiding any additional refactoring/correction as part of this PR.

No change needed. This is an exiting logic. Just moved it to a common module.
Per the comment "Queue-Service can call BLOCKED status update multiple times", both desiredStatus and currentStatus can be BLOCKED.

VonnyJap · 2024-12-12T23:06:56Z

plugins/events/create.js

@@ -244,6 +246,23 @@ module.exports = () => ({
            if (event.builds === null) {
                return boom.notFound('No jobs to start.');
            }
+
+            const virtualJobBuilds = event.builds.filter(b => b.status === 'CREATED');


is that enough to assume that all builds with status === 'CREATED' is a virtual job then?

Also, shouldn't this be virtualJobBuild (i.e., singular) as the event is triggered from a single job and there should only be 1 build created for the virtual job?

This could be plural, when we have an event started from ~pr or ~commit, and there are more than one virtual job listening to these triggers.

yes, builds created immediately after the event is created would be in CREATED status only if the job is virtual.

What about builds that are waiting for other joined parent downstream builds to complete? Aren't those builds normally created and then park with "CREATED" status?

There are the builds at the beginning of the workflow for an event. I believe, these wouldn't be waiting for any upstream.

plugins/builds/helper/updateBuild.js

VonnyJap · 2024-12-12T23:15:26Z

plugins/builds/helper/updateBuild.js

+/**
+ * Updates execution details for init step
+ * @method stopFrozenBuild
+ * @param  {Object} build       Build Object
+ * @param  {Object} app         Hapi app Object
+ */


Suggested change

/**

* Updates execution details for init step

* @method stopFrozenBuild

* @param {Object} build Build Object

* @param {Object} app Hapi app Object

*/

/**

* Updates execution details for init step

* @method updateInitStep

* @param {Object} build Build Object

* @param {Object} app Hapi app Object

* @return {Promise} Promise resolving when the init step is updated

*/

VonnyJap · 2024-12-12T23:16:20Z

plugins/builds/helper/updateBuild.js

+    step.endTime = build.startTime || new Date().toISOString();
+    step.code = 0;
+
+    return step.update();


Suggested change

return step.update();

return await step.update();

Same here we don't to await here. It is done by the caller
https://github.com/screwdriver-cd/screwdriver/pull/3252/files/d12977ba7d6469c2ca79437eb26f62eb580b3c5b#diff-2c3decb1d714adeb8f265dd68e51a120caa8ada38d91b21d9dffc66974f3276dR206

VonnyJap · 2024-12-12T23:19:42Z

plugins/builds/helper/updateBuild.js

+    if (currentStatus !== 'UNSTABLE') {
+        if (desiredStatus !== undefined) {
+            build.status = desiredStatus;
+        }
+        if (build.status === 'ABORTED') {
+            if (currentStatus === 'FROZEN') {
+                build.statusMessage = `Frozen build aborted by ${username}`;
+            } else {
+                build.statusMessage = `Aborted by ${username}`;
+            }
+        } else if (build.status === 'FAILURE' || build.status === 'SUCCESS') {
+            if (statusMessage) {
+                build.statusMessage = statusMessage;
+                build.statusMessageType = statusMessageType || null;
+            }
+        } else {
+            build.statusMessage = statusMessage || null;
+            build.statusMessageType = statusMessageType || null;
+        }
+    }


Suggested change

if (currentStatus !== 'UNSTABLE') {

if (desiredStatus !== undefined) {

build.status = desiredStatus;

}

if (build.status === 'ABORTED') {

if (currentStatus === 'FROZEN') {

build.statusMessage = `Frozen build aborted by ${username}`;

} else {

build.statusMessage = `Aborted by ${username}`;

}

} else if (build.status === 'FAILURE' || build.status === 'SUCCESS') {

if (statusMessage) {

build.statusMessage = statusMessage;

build.statusMessageType = statusMessageType || null;

}

} else {

build.statusMessage = statusMessage || null;

build.statusMessageType = statusMessageType || null;

}

}

// UNSTABLE -> SUCCESS needs to update meta and endtime.

// However, the status itself cannot be updated to SUCCESS

if (currentStatus === 'UNSTABLE') {

return;

}

// Update the build status if desiredStatus is provided

if (desiredStatus !== undefined) {

build.status = desiredStatus;

}

// Handle different statuses and set the appropriate status message

switch (build.status) {

case 'ABORTED':

build.statusMessage = currentStatus === 'FROZEN'

? `Frozen build aborted by ${username}`

: `Aborted by ${username}`;

break;

case 'FAILURE':

case 'SUCCESS':

if (statusMessage) {

build.statusMessage = statusMessage;

build.statusMessageType = statusMessageType || null;

}

break;

default:

build.statusMessage = statusMessage || null;

build.statusMessageType = statusMessageType || null;

break;

}

to improve readability:

use early return

use switch instead of nested if else.

VonnyJap · 2024-12-12T23:22:32Z

plugins/builds/helper/updateBuild.js

+    const stageJobIds = stage.jobIds;
+
+    stageJobIds.push(stage.setup);


Suggested change

const stageJobIds = stage.jobIds;

stageJobIds.push(stage.setup);

const stageJobIds = [...stage.jobIds, stage.setup];

minghay · 2024-12-12T23:56:40Z

test/plugins/events.test.js

-            eventFactoryMock.get.withArgs(parentEventId).resolves(getEventMock(testEvent));
-            eventFactoryMock.create.resolves(getEventMock(testEvent));
+            eventMock = getEventMock(testEvent);
+            // eventFactoryMock.get.withArgs(parentEventId).resolves(getEventMock(testEvent));


Suggested change

// eventFactoryMock.get.withArgs(parentEventId).resolves(getEventMock(testEvent));

minghay · 2024-12-13T00:32:00Z

plugins/events/create.js

@@ -244,6 +246,23 @@ module.exports = () => ({
            if (event.builds === null) {
                return boom.notFound('No jobs to start.');
            }
+
+            const virtualJobBuilds = event.builds.filter(b => b.status === 'CREATED');


Also, shouldn't this be virtualJobBuild (i.e., singular) as the event is triggered from a single job and there should only be 1 build created for the virtual job?

sagar1312 · 2024-12-16T17:18:05Z

@minghay @VonnyJap I have addressed the suggestions. Please take a look.

feat(3181): Skip execution of a virtual job when an event is started …

d12977b

…from that job

VonnyJap reviewed Dec 12, 2024

View reviewed changes

minghay reviewed Dec 13, 2024

View reviewed changes

feat(3181): Addressing code review suggestions

409534a

minghay approved these changes Dec 16, 2024

View reviewed changes

VonnyJap merged commit 36d3365 into master Dec 16, 2024
2 checks passed

VonnyJap deleted the sagar1312-feat-3181-skip-virtual-job-as-event-origin branch December 16, 2024 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(3181): Skip execution of a virtual job when an event is started from that job #3252

feat(3181): Skip execution of a virtual job when an event is started from that job #3252

sagar1312 commented Dec 12, 2024

coveralls commented Dec 12, 2024 •

edited

Loading

VonnyJap Dec 12, 2024

sagar1312 Dec 16, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 12, 2024

minghay Dec 13, 2024

sagar1312 Dec 16, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 16, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 12, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 12, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 12, 2024

sagar1312 Dec 16, 2024

VonnyJap Dec 12, 2024

sagar1312 Dec 16, 2024

minghay Dec 12, 2024

sagar1312 Dec 16, 2024

minghay Dec 13, 2024

sagar1312 commented Dec 16, 2024

		const stageJobIds = stage.jobIds;

		stageJobIds.push(stage.setup);

feat(3181): Skip execution of a virtual job when an event is started from that job #3252

feat(3181): Skip execution of a virtual job when an event is started from that job #3252

Conversation

sagar1312 commented Dec 12, 2024

Context

Objective

References

License

coveralls commented Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagar1312 commented Dec 16, 2024

coveralls commented Dec 12, 2024 •

edited

Loading