feat(1769): [5] Add processHooks endpoint for queue-service cooperation #2622

yk634 · 2021-12-20T08:50:38Z

Context

We implemented a worker that retrieves a queue and requests the API last time.
I add an endpoint that will receive the webhookConfig sent by the worker.

The scope of this PR is to implement commonly referenced helpers.
The process of calling the quese-service API will be implemented in another context.

Objective

Add an endpoint /processHooks that receives the webhookConfig sent by the worker and starts the event.
The parts that share processing with /webhooks are shared as helper.
Some of the tests done by the webhooks plugin have been moved to helper tests.
- We should migrate most of the tests run at the webhooks plugin to the helper, but in this PR we have only migrated the tests that are easy to migrate. The rest of the migration will be handled in another context.
- There is no change in the tests run, so the all necessary test cases will still be run.

References

Please do not merge this PR until after the following PRs have been merged.

#1769 (comment)

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 · 2021-12-20T08:58:58Z

Coverage decreased (-0.08%) to 95.482% when pulling 6280e07 on sonic-screwdriver-cd:use-queueservice-for-webhook into 2f0f5d9 on screwdriver-cd:master.

yoshwata · 2021-12-20T21:07:04Z

If you change the key name of payload, the getChangedFiles is also need to be changed.

screwdriver/plugins/events/create.js

Line 149 in 6f83ce1

scm.getChangedFiles({

plugins/processHooks/index.js

tk3fftk · 2021-12-21T01:35:00Z

plugins/processHooks/index.js

+const joi = require('joi');
+const { startHookEvent } = require('../webhooks/helper');
+
+const DEFAULT_MAX_BYTES = 1048576;


is this used?

This is not used. I removed them.

plugins/processHooks/index.js

tk3fftk · 2021-12-21T01:51:56Z

plugins/webhooks/index.js

-                            // disregard skip ci for pull request events
-                            return pullRequestEvent(pluginOptions, request, h, parsed, token);
-                        }
+                        return await startHookEvent(pluginOptions, request, h, parsed);


I don't know well about scope of this PR. Can we start event directly instead of calling queue service API when /webhook is called?

Yes. The scope of this PR is to implement commonly referenced helpers.
The process of calling the quese-service API will be implemented in another context.

Co-authored-by: Hiroki Takatsuka <3445553+tk3fftk@users.noreply.github.com>

…ic-screwdriver-cd/screwdriver into use-queueservice-for-webhook

wahapo · 2021-12-21T06:56:33Z

plugins/webhooks/index.js

-                            // disregard skip ci for pull request events
-                            return pullRequestEvent(pluginOptions, request, h, parsed, token);
-                        }
+                        return await startHookEvent(pluginOptions, request, h, parsed);


Since pluginOptions is stored in parsed.pluginOptions, shouldn't 3 arguments, request, h, parsed, be enough?

I fixed it.

wahapo · 2021-12-21T07:51:19Z

plugins/webhooks/helper.js

+ * @method startHookEvent
+ * @param  {Hapi.request} request   Request from user
+ * @param  {Object} h               Response toolkit
+ * @param  {Object} webhookConfig   Parsed request.payload by scm webhook


Does it store pluginOptions as well as request.payload?
In the requestHook API, it's not passing webhookConfig.pluginOptions as an argument, is that ok?

Yes. I fixed the documentation of startHookEvent.

tk3fftk

I think we should add and migrate some test for helper.js from webhooks.test.js because this PR separates function and another PR will make some current webhooks test outdated.

tk3fftk · 2021-12-21T08:01:17Z

plugins/webhooks/helper.js

+}
+
+/**
+ * Start pipeline events from scm webhook config


same here but I don't know the same is correct.

Suggested change

* Start pipeline events from scm webhook config

* Start pipeline events with scm webhook config via queue-service

I fixed it.
Since this function is not necessarily called via queue-service, the phrase via queue-service has been omitted.

tk3fftk · 2021-12-21T08:14:50Z

plugins/webhooks/helper.js

+                scmUri = await scm.parseUrl({ checkoutUrl, token, scmContext });
+            }
+            webhookConfig.changedFiles = await scm.getChangedFiles({
+                webhookConfig,


this might not be able to omit because the name is different.

Suggested change

webhookConfig,

payload: webhookConfig,

and does webhookConfig contain request.payload?

The argument of getChangedFiles will turn into webhookConfig at this PR.

tk3fftk · 2021-12-21T08:21:06Z

plugins/webhooks/helper.js

+
+        if (type === 'pr') {
+            // disregard skip ci for pull request events
+            return pullRequestEvent(webhookConfig.pluginOptions, request, h, webhookConfig, token);


Suggested change

return pullRequestEvent(webhookConfig.pluginOptions, request, h, webhookConfig, token);

return pullRequestEvent(pluginOptions: webhookConfig.pluginOptions, request, h, webhookConfig, token);

Since the argument of this function is not an object literal, it should be left as it is.

yk634 · 2021-12-23T02:57:25Z

In addition, some of the tests run by the webhooks plugin have been moved to helper tests.

feat(1769): Add processHooks endpoint for queue-service cooperation

1f875e3

Merge branch 'master' into use-queueservice-for-webhook

1bb3895

yk634 changed the title ~~[WIP] feat(1769): [4] Add processHooks endpoint for queue-service cooperation~~ feat(1769): [4] Add processHooks endpoint for queue-service cooperation Dec 20, 2021

yoshwata reviewed Dec 20, 2021

View reviewed changes

plugins/processHooks/index.js Show resolved Hide resolved

yoshwata reviewed Dec 21, 2021

View reviewed changes

plugins/processHooks/index.js Show resolved Hide resolved

tk3fftk reviewed Dec 21, 2021

View reviewed changes

yk634 mentioned this pull request Dec 21, 2021

feat(1769): [2] Change data structure for queue-service cooperation. screwdriver-cd/scm-base#90

Merged

yk634 changed the title ~~feat(1769): [4] Add processHooks endpoint for queue-service cooperation~~ feat(1769): [5] Add processHooks endpoint for queue-service cooperation Dec 21, 2021

yk634 and others added 4 commits December 21, 2021 13:17

feat(1769): fix api documentation of processHooks.

c15c6a9

Co-authored-by: Hiroki Takatsuka <3445553+tk3fftk@users.noreply.github.com>

feat(1769): Fix for comments.

03299cd

Merge branch 'use-queueservice-for-webhook' of https://github.com/son…

e069add

…ic-screwdriver-cd/screwdriver into use-queueservice-for-webhook

feat(1769): Fix for comments.

33d11c7

wahapo reviewed Dec 21, 2021

View reviewed changes

Yuta Kaneda added 2 commits December 21, 2021 16:21

feat(1769): Fix returning 204 if parsed is null.

0a33817

feat(1769): Fix for comment.

e36727d

itleigns previously approved these changes Dec 21, 2021

View reviewed changes

wahapo reviewed Dec 21, 2021

View reviewed changes

feat(1769): fix function documentation of startHookEvent.

9ea587b

yk634 dismissed itleigns’s stale review via 9ea587b December 21, 2021 08:12

tk3fftk reviewed Dec 21, 2021

View reviewed changes

feat(1769): Fix for comment.

cdc7f19

wahapo previously approved these changes Dec 21, 2021

View reviewed changes

Merge branch 'master' into use-queueservice-for-webhook

8e1920a

tk3fftk previously approved these changes Dec 22, 2021

View reviewed changes

yk634 changed the title ~~feat(1769): [5] Add processHooks endpoint for queue-service cooperation~~ [DNM] feat(1769): [5] Add processHooks endpoint for queue-service cooperation Dec 22, 2021

yk634 changed the title ~~[DNM] feat(1769): [5] Add processHooks endpoint for queue-service cooperation~~ feat(1769): [5] Add processHooks endpoint for queue-service cooperation Dec 22, 2021

feat(1769): transfer the test from webhooks to webhooks.helper

6280e07

yk634 dismissed stale reviews from tk3fftk and wahapo via 6280e07 December 23, 2021 01:58

yk634 mentioned this pull request Dec 23, 2021

Tests that should be run as helper tests are being run as webhooks plugin tests. #2625

Closed

kumada626 approved these changes Dec 24, 2021

View reviewed changes

ibu1224 approved these changes Dec 24, 2021

View reviewed changes

yoshwata approved these changes Dec 24, 2021

View reviewed changes

tkyi merged commit 63d19a4 into screwdriver-cd:master Jan 3, 2022

tk3fftk deleted the use-queueservice-for-webhook branch January 11, 2022 02:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(1769): [5] Add processHooks endpoint for queue-service cooperation #2622

feat(1769): [5] Add processHooks endpoint for queue-service cooperation #2622

yk634 commented Dec 20, 2021 •

edited

Loading

coveralls commented Dec 20, 2021 •

edited

Loading

yoshwata commented Dec 20, 2021

tk3fftk Dec 21, 2021

yk634 Dec 21, 2021

tk3fftk Dec 21, 2021

yk634 Dec 21, 2021

wahapo Dec 21, 2021

yk634 Dec 21, 2021

wahapo Dec 21, 2021

yk634 Dec 21, 2021

tk3fftk left a comment

tk3fftk Dec 21, 2021

yk634 Dec 21, 2021

tk3fftk Dec 21, 2021

tk3fftk Dec 21, 2021

yk634 Dec 21, 2021 •

edited

Loading

tk3fftk Dec 21, 2021

tk3fftk Dec 21, 2021

yk634 Dec 21, 2021

yk634 commented Dec 23, 2021

	* Start pipeline events from scm webhook config
	* Start pipeline events with scm webhook config via queue-service

	return pullRequestEvent(webhookConfig.pluginOptions, request, h, webhookConfig, token);
	return pullRequestEvent(pluginOptions: webhookConfig.pluginOptions, request, h, webhookConfig, token);

feat(1769): [5] Add processHooks endpoint for queue-service cooperation #2622

feat(1769): [5] Add processHooks endpoint for queue-service cooperation #2622

Conversation

yk634 commented Dec 20, 2021 • edited Loading

Context

Objective

References

License

coveralls commented Dec 20, 2021 • edited Loading

yoshwata commented Dec 20, 2021

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

tk3fftk left a 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

yk634 Dec 21, 2021 • 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

yk634 commented Dec 23, 2021

yk634 commented Dec 20, 2021 •

edited

Loading

coveralls commented Dec 20, 2021 •

edited

Loading

yk634 Dec 21, 2021 •

edited

Loading