Correct notification routing for `DEPLOYMENT_STARTED` by JohnTitor · Pull Request #5523 · pipe-cd/pipecd

JohnTitor · 2025-01-31T04:27:41Z

What this PR does:

This fixes the issue the DEPLOYMENT_STARTED event notification is routed unconditionally.

Why we need it:

Without this, users would receive that event notification even if they don't want.

Which issue(s) this PR fixes:

None

Does this PR introduce a user-facing change?: Yes

How are users affected by this change: Users can control the notification correctly.
Is this breaking change: No
How to migrate (if breaking change): N/A

codecov · 2025-01-31T04:41:24Z

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 26.43%. Comparing base (6f4d443) to head (fd6ce23).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/model/notificationevent.go	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5523      +/-   ##
==========================================
- Coverage   26.43%   26.43%   -0.01%     
==========================================
  Files         464      464              
  Lines       49786    49790       +4     
==========================================
  Hits        13163    13163              
- Misses      35569    35573       +4     
  Partials     1054     1054

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

t-kikuc

~~LTGM~~ LGreatTM
Thank you so much!!!!

Warashi

Thank you for your contribution!
I commented on missing test cases; please check them.

Warashi · 2025-01-31T05:17:09Z

pkg/app/piped/notifier/matcher_test.go

+				{
+					Type: model.NotificationEventType_EVENT_DEPLOYMENT_STARTED,
+					Metadata: &model.NotificationEventDeploymentStarted{
+						Deployment: &model.Deployment{
+							ApplicationName: "canary",
+						},
+					},
+				}: true,


Could you please add a test case to return false?

Suggested change

{

Type: model.NotificationEventType_EVENT_DEPLOYMENT_STARTED,

Metadata: &model.NotificationEventDeploymentStarted{

Deployment: &model.Deployment{

ApplicationName: "canary",

},

},

}: true,

{

Type: model.NotificationEventType_EVENT_DEPLOYMENT_STARTED,

Metadata: &model.NotificationEventDeploymentStarted{

Deployment: &model.Deployment{

ApplicationName: "canary",

},

},

}: true,

{

Type: model.NotificationEventType_EVENT_DEPLOYMENT_STARTED,

Metadata: &model.NotificationEventDeploymentStarted{

Deployment: &model.Deployment{

ApplicationName: "bluegreen",

},

},

}: false,

Sure, added in edfb792 (#5523).

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

Warashi

LGTM!
Thank you for a great contribution!

* Correct notification routing for `DEPLOYMENT_STARTED` Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> * Harden test case Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> --------- Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com>

* Correct notification routing for `DEPLOYMENT_STARTED` (#5523) * Correct notification routing for `DEPLOYMENT_STARTED` Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> * Harden test case Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> --------- Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Sort results of plan-preview (#5540) * Sort results of plan-preview Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * Ensure the order of list piped Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * fix: lint Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * fix: move sorting to pipectl Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * fix: add testcase Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * fix: dev docs Signed-off-by: kj455 <kaji.ibuki45@gmail.com> * add docs Signed-off-by: kj455 <kaji.ibuki45@gmail.com> --------- Signed-off-by: kj455 <kaji.ibuki45@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Enhanced EventWatcher logs (#5558) * Show push error log earlier than reporting Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Use WarnLog in retry Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * clarify log messages Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * clarify log messages Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * add TestDoCalls for asserting counts Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * add eventIDs in log Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * enrich logs in updateValues Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * nits Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * Revert "add TestDoCalls for asserting counts" This reverts commit de3f112. Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> --------- Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * update RELEASE to v0.50.2 with doc update (#5571) Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> --------- Signed-off-by: Yuki Okushi <okushi@canary-inc.jp> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> Signed-off-by: kj455 <kaji.ibuki45@gmail.com> Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Co-authored-by: Yuki Okushi <okushi@canary-inc.jp> Co-authored-by: Ibuki Kaji <38521709+kj455@users.noreply.github.com> Co-authored-by: Tetsuya KIKUCHI <97105818+t-kikuc@users.noreply.github.com>

t-kikuc · 2025-02-26T07:46:52Z

@JohnTitor

Hi, we released your PR in v0.50.2 last week! Thank you so much!

https://github.com/pipe-cd/pipecd/releases/tag/v0.50.2

JohnTitor requested review from Warashi, ffjlabo, khanhtc1202 and t-kikuc as code owners January 31, 2025 04:27

github-actions bot added the area/go label Jan 31, 2025

JohnTitor force-pushed the fix/route-deployment-started branch from bfdb901 to fd6ce23 Compare January 31, 2025 04:28

Correct notification routing for DEPLOYMENT_STARTED

2296b0c

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

JohnTitor force-pushed the fix/route-deployment-started branch from fd6ce23 to 2296b0c Compare January 31, 2025 04:47

t-kikuc previously approved these changes Jan 31, 2025

View reviewed changes

t-kikuc enabled auto-merge (squash) January 31, 2025 05:07

t-kikuc added cherry-pick v0.50.2 labels Jan 31, 2025

Warashi reviewed Jan 31, 2025

View reviewed changes

auto-merge was automatically disabled January 31, 2025 05:21
Head branch was pushed to by a user without write access

JohnTitor dismissed t-kikuc’s stale review via 1ebc304 January 31, 2025 05:22

Harden test case

edfb792

Signed-off-by: Yuki Okushi <okushi@canary-inc.jp>

JohnTitor force-pushed the fix/route-deployment-started branch from 1ebc304 to edfb792 Compare January 31, 2025 05:22

t-kikuc enabled auto-merge (squash) January 31, 2025 05:23

Warashi approved these changes Jan 31, 2025

View reviewed changes

t-kikuc approved these changes Jan 31, 2025

View reviewed changes

t-kikuc merged commit 3021db1 into pipe-cd:master Jan 31, 2025
11 checks passed

JohnTitor deleted the fix/route-deployment-started branch January 31, 2025 05:28

Warashi mentioned this pull request Jan 31, 2025

Correct notification route handling for stage events #5524

Merged

github-actions bot mentioned this pull request Feb 17, 2025

update RELEASE to v0.50.2 #5571

Merged

github-actions bot mentioned this pull request Feb 17, 2025

Cherry-pick #5523 #5540 #5558 #5572

Closed

github-actions bot mentioned this pull request Feb 17, 2025

Cherry-pick #5523 #5540 #5558 #5571 #5573

Merged

github-actions bot mentioned this pull request Mar 14, 2025

Release version v0.50.3 #5665

Closed

github-actions bot mentioned this pull request Mar 25, 2025

Release v0.51.0 #5695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct notification routing for `DEPLOYMENT_STARTED`#5523

Correct notification routing for `DEPLOYMENT_STARTED`#5523
t-kikuc merged 2 commits intopipe-cd:masterfrom
JohnTitor:fix/route-deployment-started

JohnTitor commented Jan 31, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jan 31, 2025

Uh oh!

t-kikuc left a comment •

edited

Loading

Uh oh!

Warashi left a comment

Uh oh!

Warashi Jan 31, 2025

Uh oh!

JohnTitor Jan 31, 2025

Uh oh!

Warashi left a comment

Uh oh!

Uh oh!

t-kikuc commented Feb 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

JohnTitor commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 31, 2025

Codecov Report

Uh oh!

t-kikuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Warashi Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

JohnTitor Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

t-kikuc commented Feb 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohnTitor commented Jan 31, 2025 •

edited

Loading

t-kikuc left a comment •

edited

Loading