fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline #3095

VonnyJap · 2024-04-23T16:48:52Z

Context

The feature of subscribed-scm-notifications is defective as such that:

when a repo is being subscribed by another downstream pipeline, the PR job never start
a downstream pipeline is triggered regardless of event types of the upstream repo, for example supposed this is the config

subscribe:
    scmUrls:
        - git@github.com:supra08/functional-workflow.git: ['~pr']
 jobs:
    A:
        steps:
            - echo: echo test
        requires: [~pr, ~commit, ~subscribe]

With the current implementation, job A is triggered even during the commit event of git@github.com:supra08/functional-workflow.git.

Objective

The fix includes:

fixing the original SHA of the upstream pipeline that was accidentally overwritten by the subscribed config during the PR events
determine the event types of the upstream repo and do a match to the event types that the downstream pipeline is subscribing to

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.

coveralls · 2024-04-23T17:51:56Z

coverage: 95.198% (-0.09%) from 95.29%
when pulling b8e2b94 on subscribe
into a20a5fb on master.

pritamstyz4ever · 2024-04-30T13:38:49Z

plugins/webhooks/helper.js

+        p.subscribedScmUrlsWithActions.forEach(subscribedScmUriWithAction => {
+            const { scmUri: subscribedScmUri, actions: subscribedActions } = subscribedScmUriWithAction;
+
+            if (pipelinesOnCommitBranch[0].scmUri === subscribedScmUri) {


should check length of arr when selecting index

these lines should be sufficient to ensure that pipelinesOnCommitBranch at least is a length of 1.

screwdriver/plugins/webhooks/helper.js

Lines 367 to 369 in c3d7ce4

if (pipelinesOnCommitBranch.length === 0) {

return currentRepoPipelines;

}

and I have also added a comment that the subsequent block will only proceed when pipelinesOnCommitBranch is a length of at least 1.

screwdriver/plugins/webhooks/helper.js

Lines 371 to 372 in c3d7ce4

// process the pipelinesWithSubscribedRepos only when the pipelinesOnCommitBranch is not empty

// pipelinesOnCommitBranch has the information to determine the triggering event of downstream subscribing repo

pritamstyz4ever · 2024-04-30T13:48:11Z

plugins/webhooks/helper.js

                if (uriTrimmer(scmConfig.scmUri) !== uriTrimmer(p.scmUri)) {
                    subscribedConfigSha = sha;

                    try {
-                        sha = await pipelineFactory.scm.getCommitSha({
+                        configPipelineSha = await pipelineFactory.scm.getCommitSha({


configPipelineSha is not defined

it is initialized

screwdriver/plugins/webhooks/helper.js

Line 497 in c3d7ce4

let configPipelineSha = '';

pritamstyz4ever · 2024-04-30T13:55:35Z

plugins/webhooks/helper.js

+    if (pipelinesOnCommitBranch.length === 0) {
+        return currentRepoPipelines;
+    }
+
+    // process the pipelinesWithSubscribedRepos only when the pipelinesOnCommitBranch is not empty
+    // pipelinesOnCommitBranch has the information to determine the triggering event of downstream subscribing repo
+    pipelinesWithSubscribedRepos.forEach(p => {
+        if (!Array.isArray(p.subscribedScmUrlsWithActions)) {
+            return;
+        }
+        p.subscribedScmUrlsWithActions.forEach(subscribedScmUriWithAction => {
+            const { scmUri: subscribedScmUri, actions: subscribedActions } = subscribedScmUriWithAction;
+
+            if (pipelinesOnCommitBranch[0].scmUri === subscribedScmUri) {


we could probably refactor the loops into a function and return the filteredPipelines

I think as it stands, there is no reusable purpose for this section that can bring a benefit of putting into a function. So I would propose to keep it as is.

pritamstyz4ever

some comments

VonnyJap · 2024-05-02T22:32:38Z

@pritamstyz4ever - responded to your comments.

pritamstyz4ever · 2024-05-03T19:31:28Z

test/plugins/webhooks.helper.test.js

+                            branch: Promise.resolve('master')
+                        })
+                    ]);
+                const pipelineMock2 = _.cloneDeep(pipelineMock);


you can write a simple deep clone instead of adding the entire lodash pkg I think

pritamstyz4ever · 2024-05-03T19:32:58Z

package.json

@@ -93,6 +93,7 @@
    "js-yaml": "^3.14.1",
    "jsonwebtoken": "^9.0.0",
    "license-checker": "^25.0.1",
+    "lodash": "^4.17.21",


used only in tests, move to dev-dependencies

VonnyJap changed the title ~~fix:~~ fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline Apr 23, 2024

VonnyJap requested review from sagar1312 and pritamstyz4ever April 29, 2024 23:00

pritamstyz4ever reviewed Apr 30, 2024

View reviewed changes

VonnyJap added 8 commits May 2, 2024 15:28

fix the sha for subscribed config

68bed52

fix the logic of which event should trigger subscribe

40a721f

fixing tests for the webhooks helper

34baf69

add functional tests for subscribe config

8576368

add functional tests for subscribe config

67e7823

fixed unit test for webhooks helper

5ca46ab

removed some comments

d305a7f

fix lint

b8e2b94

VonnyJap force-pushed the subscribe branch from 238b624 to b8e2b94 Compare May 2, 2024 22:31

pritamstyz4ever reviewed May 3, 2024

View reviewed changes

pritamstyz4ever approved these changes May 3, 2024

View reviewed changes

VonnyJap merged commit 0782760 into master May 3, 2024
2 checks passed

VonnyJap deleted the subscribe branch May 3, 2024 20:30

VonnyJap mentioned this pull request May 4, 2024

fix: no lodash for cloning #3111

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline #3095

fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline #3095

VonnyJap commented Apr 23, 2024 •

edited

Loading

coveralls commented Apr 23, 2024 •

edited

Loading

pritamstyz4ever Apr 30, 2024 •

edited

Loading

VonnyJap May 2, 2024

VonnyJap May 2, 2024

pritamstyz4ever Apr 30, 2024

VonnyJap May 2, 2024

pritamstyz4ever Apr 30, 2024

VonnyJap May 2, 2024

pritamstyz4ever left a comment

VonnyJap commented May 2, 2024

pritamstyz4ever May 3, 2024

pritamstyz4ever May 3, 2024

	if (pipelinesOnCommitBranch.length === 0) {
	return currentRepoPipelines;
	}

	// process the pipelinesWithSubscribedRepos only when the pipelinesOnCommitBranch is not empty
	// pipelinesOnCommitBranch has the information to determine the triggering event of downstream subscribing repo

fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline #3095

fix: subscribed-scm-notifications should respect the event types and allow PR job of upstream pipeline #3095

Conversation

VonnyJap commented Apr 23, 2024 • edited Loading

Context

Objective

License

coveralls commented Apr 23, 2024 • edited Loading

pritamstyz4ever Apr 30, 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

pritamstyz4ever left a comment

Choose a reason for hiding this comment

VonnyJap commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VonnyJap commented Apr 23, 2024 •

edited

Loading

coveralls commented Apr 23, 2024 •

edited

Loading

pritamstyz4ever Apr 30, 2024 •

edited

Loading