Skip commit and push when no replacement happens in EventWatcher #5310

t-kikuc · 2024-11-05T10:50:56Z

What this PR does:

Fix the following bug in EventWatcher by handling no-diff.

Why we need it:

Without this PR, when no diff occurs by an event, the event will fail.

Before fix: Failed to push changed files: err: exit status 128, out: fatal: invalid refspec ''

After fix: Nothing to commit

The cause is that when no diff occurs, branchName will be nil as below.

pipecd/pkg/app/piped/eventwatcher/eventwatcher.go

Lines 643 to 645 in d7f4369

    
           if len(changes) == 0 { 
        
           	return "", nil 
        
           }

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: t-kikuc <[email protected]>

codecov · 2024-11-05T10:55:13Z

Codecov Report

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

Project coverage is 25.26%. Comparing base (d7f4369) to head (7b155e6).
Report is 468 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/app/piped/eventwatcher/eventwatcher.go	0.00%	18 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5310      +/-   ##
==========================================
+ Coverage   25.23%   25.26%   +0.03%     
==========================================
  Files         444      444              
  Lines       47480    47528      +48     
==========================================
+ Hits        11982    12010      +28     
- Misses      34556    34575      +19     
- Partials      942      943       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

khanhtc1202

Got your point, thanks 👍

ffjlabo

Thank you for the imporvement! I left an imo comment

ffjlabo · 2024-11-06T08:30:19Z

pkg/app/piped/eventwatcher/eventwatcher.go

@@ -388,7 +391,7 @@ func (w *watcher) execute(ctx context.Context, repo git.Repo, repoID string, eve
 			switch handler.Type {
 			case config.EventWatcherHandlerTypeGitUpdate:
 				branchName, err := w.commitFiles(ctx, latestEvent, matcher.Name, handler.Config.CommitMessage, e.GitPath, handler.Config.Replacements, tmpRepo, handler.Config.MakePullRequest)
-				if err != nil {
+				if err != nil && !errors.Is(err, errNoChanges) {


[IMO] How about using the bool variable like change to minimize the scope of the error variable and also for the readability?

Suggested change

if err != nil && !errors.Is(err, errNoChanges) {

noChange := false

if err != nil {

//

if errors.Is(err, errNoChanges) {

noChange = true

} else {

...

continue

}

}

if noChange {

handledEvent.StatusDescription = "Nothing to commit"

gitNoChangeEvents = append(gitNoChangeEvents, handledEvent)

} else {

thank you , got it, i fixed in 7b155e6

Signed-off-by: t-kikuc <[email protected]>

ffjlabo

LGTM

khanhtc1202

LGTM

* fix: skip push if no diff exist Signed-off-by: t-kikuc <[email protected]> * modify message Signed-off-by: t-kikuc <[email protected]> * refactor: use noChange variable Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Signed-off-by: pipecd-bot <[email protected]>

* Add event context (#5295) * Add contexts to the RegisterEventRequest Signed-off-by: Yoshiki Fujikane <[email protected]> * Add contexts to model.Event Signed-off-by: Yoshiki Fujikane <[email protected]> * Store event context in Control Plane Signed-off-by: Yoshiki Fujikane <[email protected]> * Add trailers when commiting on event watcher Signed-off-by: Yoshiki Fujikane <[email protected]> * Fix for failed build Signed-off-by: Yoshiki Fujikane <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Add docs for pipectl event register --contexts on the event watcher usage page (#5299) * Add docs for pipectl event register --contexts on the event watcher usage page Signed-off-by: Yoshiki Fujikane <[email protected]> * Fix docs Signed-off-by: Yoshiki Fujikane <[email protected]> * Fix command Signed-off-by: Yoshiki Fujikane <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Clone manifests not to modify original manifests (#5306) Signed-off-by: t-kikuc <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Lambda: clone manifests not to modify original manifests (#5308) Signed-off-by: t-kikuc <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Skip commit and push when no replacement happens in EventWatcher (#5310) * fix: skip push if no diff exist Signed-off-by: t-kikuc <[email protected]> * modify message Signed-off-by: t-kikuc <[email protected]> * refactor: use noChange variable Signed-off-by: t-kikuc <[email protected]> --------- Signed-off-by: t-kikuc <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Execute git clean partially when drift detection for every app is done (#5312) * Execute git clean partially when drift detection for every app is done Signed-off-by: Yoshiki Fujikane <[email protected]> * Rename to CleanPath Signed-off-by: Yoshiki Fujikane <[email protected]> * Add test for the outside dir pattern Signed-off-by: Yoshiki Fujikane <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> * Update RELEASE and docs to v0.49.3 (#5315) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> --------- Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: pipecd-bot <[email protected]> Signed-off-by: t-kikuc <[email protected]> Co-authored-by: Yoshiki Fujikane <[email protected]> Co-authored-by: Tetsuya Kikuchi <[email protected]>

t-kikuc added 2 commits November 5, 2024 19:35

fix: skip push if no diff exist

456119c

Signed-off-by: t-kikuc <[email protected]>

modify message

723393b

Signed-off-by: t-kikuc <[email protected]>

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and Warashi as code owners November 5, 2024 10:50

github-actions bot added the area/go label Nov 5, 2024

t-kikuc added the kind/bug Something isn't working as expected label Nov 6, 2024

khanhtc1202 previously approved these changes Nov 6, 2024

View reviewed changes

ffjlabo reviewed Nov 6, 2024

View reviewed changes

refactor: use noChange variable

7b155e6

Signed-off-by: t-kikuc <[email protected]>

t-kikuc dismissed khanhtc1202’s stale review via 7b155e6 November 6, 2024 08:58

t-kikuc enabled auto-merge (squash) November 6, 2024 08:59

ffjlabo approved these changes Nov 6, 2024

View reviewed changes

khanhtc1202 approved these changes Nov 6, 2024

View reviewed changes

t-kikuc merged commit 5898f42 into master Nov 6, 2024
17 of 18 checks passed

t-kikuc deleted the fix-eventwatcher-nochange branch November 6, 2024 11:48

ffjlabo added cherry-pick v0.49.3 labels Nov 7, 2024

github-actions bot mentioned this pull request Nov 7, 2024

Update RELEASE and docs to v0.49.3 #5315

Merged

github-actions bot mentioned this pull request Nov 7, 2024

Cherry-pick #5295 #5299 #5306 #5308 #5310 #5312 #5315 #5316

Merged

github-actions bot mentioned this pull request Nov 20, 2024

Update RELEASE to v0.49.4 #5351

Merged

github-actions bot mentioned this pull request Dec 3, 2024

Make release version=v0.50.0 #5382

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Skip commit and push when no replacement happens in EventWatcher #5310

Skip commit and push when no replacement happens in EventWatcher #5310

Uh oh!

t-kikuc commented Nov 5, 2024

Uh oh!

codecov bot commented Nov 5, 2024 •

edited

Loading

Uh oh!

khanhtc1202 left a comment

Uh oh!

ffjlabo left a comment

Uh oh!

ffjlabo Nov 6, 2024

Uh oh!

t-kikuc Nov 6, 2024

Uh oh!

ffjlabo left a comment

Uh oh!

khanhtc1202 left a comment

Uh oh!

Uh oh!

Uh oh!

-				if err != nil && !errors.Is(err, errNoChanges) {
+				noChange := false
+				if err != nil {
+				    //
+				    if errors.Is(err, errNoChanges) {
+				        noChange = true
+				    } else {
+				     ...
+				     continue
+				    }
+				}
+				if noChange {
+				        handledEvent.StatusDescription = "Nothing to commit"
+					gitNoChangeEvents = append(gitNoChangeEvents, handledEvent)
+				} else {

Skip commit and push when no replacement happens in EventWatcher #5310

Skip commit and push when no replacement happens in EventWatcher #5310

Uh oh!

Conversation

t-kikuc commented Nov 5, 2024

Uh oh!

codecov bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

ffjlabo left a comment

Choose a reason for hiding this comment

Uh oh!

ffjlabo Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

t-kikuc Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

ffjlabo left a comment

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Nov 5, 2024 •

edited

Loading