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 <tkikuchi07f@gmail.com>

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 4 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.

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 <tkikuchi07f@gmail.com>

ffjlabo

LGTM

khanhtc1202

LGTM

* fix: skip push if no diff exist Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * modify message Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * refactor: use noChange variable 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>

* Add event context (#5295) * Add contexts to the RegisterEventRequest Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add contexts to model.Event Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Store event context in Control Plane Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add trailers when commiting on event watcher Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix for failed build Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * 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 <ffjlabo@gmail.com> * Fix docs Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Fix command Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Clone manifests not to modify original manifests (#5306) Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Lambda: clone manifests not to modify original manifests (#5308) Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Skip commit and push when no replacement happens in EventWatcher (#5310) * fix: skip push if no diff exist Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * modify message Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> * refactor: use noChange variable 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> * 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 <ffjlabo@gmail.com> * Rename to CleanPath Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> * Add test for the outside dir pattern Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> * Update RELEASE and docs to v0.49.3 (#5315) Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> --------- Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com> Signed-off-by: pipecd-bot <pipecd.dev@gmail.com> Signed-off-by: t-kikuc <tkikuchi07f@gmail.com> Co-authored-by: Yoshiki Fujikane <40124947+ffjlabo@users.noreply.github.com> Co-authored-by: Tetsuya Kikuchi <97105818+t-kikuc@users.noreply.github.com>

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

fix: skip push if no diff exist

456119c

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

modify message

723393b

Signed-off-by: t-kikuc <tkikuchi07f@gmail.com>

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 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 <tkikuchi07f@gmail.com>

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

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

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

t-kikuc commented Nov 5, 2024

codecov bot commented Nov 5, 2024 •

edited

Loading

khanhtc1202 left a comment

ffjlabo left a comment

ffjlabo Nov 6, 2024

t-kikuc Nov 6, 2024

ffjlabo left a comment

khanhtc1202 left a comment

-				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

Conversation

t-kikuc commented Nov 5, 2024

codecov bot commented Nov 5, 2024 • edited Loading

Codecov Report

khanhtc1202 left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

ffjlabo Nov 6, 2024

Choose a reason for hiding this comment

t-kikuc Nov 6, 2024

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 5, 2024 •

edited

Loading