Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 6, 2024
Merged

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
456119c
fix: skip push if no diff exist
t-kikuc Nov 5, 2024
723393b
modify message
t-kikuc Nov 5, 2024
7b155e6
refactor: use noChange variable
t-kikuc Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions pkg/app/piped/eventwatcher/eventwatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
numToMakeOutdated = 10
)

var errNoChanges = errors.New("nothing to commit")

type Watcher interface {
Run(context.Context) error
}
Expand Down Expand Up @@ -336,6 +338,7 @@
outDatedDuration = time.Hour
gitUpdateEvent = false
branchHandledEvents = make(map[string][]*pipedservice.ReportEventStatusesRequest_Event, len(eventCfgs))
gitNoChangeEvents = make([]*pipedservice.ReportEventStatusesRequest_Event, 0)

Check warning on line 341 in pkg/app/piped/eventwatcher/eventwatcher.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/eventwatcher/eventwatcher.go#L341 

Added line #L341 was not covered by tests
)
for _, e := range eventCfgs {
for _, cfg := range e.Configs {
Expand Down Expand Up @@ -388,7 +391,7 @@
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) {

Check warning on line 394 in pkg/app/piped/eventwatcher/eventwatcher.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/eventwatcher/eventwatcher.go#L394 

Added line #L394 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you , got it, i fixed in 7b155e6

w.logger.Error("failed to commit outdated files", zap.Error(err))
handledEvent := &pipedservice.ReportEventStatusesRequest_Event{
Id: latestEvent.Id,
Expand All @@ -398,12 +401,18 @@
branchHandledEvents[branchName] = append(branchHandledEvents[branchName], handledEvent)
continue
}

handledEvent := &pipedservice.ReportEventStatusesRequest_Event{
Id: latestEvent.Id,
Status: model.EventStatus_EVENT_SUCCESS,
StatusDescription: fmt.Sprintf("Successfully updated %d files in the %q repository", len(handler.Config.Replacements), repoID),
Id: latestEvent.Id,
Status: model.EventStatus_EVENT_SUCCESS,
}
if errors.Is(err, errNoChanges) {
handledEvent.StatusDescription = "Nothing to commit"
gitNoChangeEvents = append(gitNoChangeEvents, handledEvent)
} else {
handledEvent.StatusDescription = fmt.Sprintf("Successfully updated %d files in the %q repository", len(handler.Config.Replacements), repoID)
branchHandledEvents[branchName] = append(branchHandledEvents[branchName], handledEvent)

Check warning on line 414 in pkg/app/piped/eventwatcher/eventwatcher.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/eventwatcher/eventwatcher.go#L406-L414 

Added lines #L406 - L414 were not covered by tests
}
branchHandledEvents[branchName] = append(branchHandledEvents[branchName], handledEvent)
if latestEvent.CreatedAt > maxTimestamp {
maxTimestamp = latestEvent.CreatedAt
}
Expand All @@ -428,6 +437,13 @@
return nil
}

if len(gitNoChangeEvents) > 0 {
if _, err := w.apiClient.ReportEventStatuses(ctx, &pipedservice.ReportEventStatusesRequest{Events: gitNoChangeEvents}); err != nil {
w.logger.Error("failed to report event statuses", zap.Error(err))
}
w.executionMilestoneMap.Store(repoID, maxTimestamp)

Check warning on line 444 in pkg/app/piped/eventwatcher/eventwatcher.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/eventwatcher/eventwatcher.go#L440-L444 

Added lines #L440 - L444 were not covered by tests
}

var responseError error
retry := backoff.NewRetry(retryPushNum, backoff.NewConstant(retryPushInterval))
for branch, events := range branchHandledEvents {
Expand Down Expand Up @@ -603,6 +619,7 @@
}

// commitFiles commits changes if the data in Git is different from the latest event.
// If there are no changes to commit, it returns errNoChanges.
func (w *watcher) commitFiles(ctx context.Context, latestEvent *model.Event, eventName, commitMsg, gitPath string, replacements []config.EventWatcherReplacement, repo git.Repo, newBranch bool) (string, error) {
// Determine files to be changed by comparing with the latest event.
changes := make(map[string][]byte, len(replacements))
Expand Down Expand Up @@ -641,7 +658,7 @@
changes[filePath] = newContent
}
if len(changes) == 0 {
return "", nil
return "", errNoChanges

Check warning on line 661 in pkg/app/piped/eventwatcher/eventwatcher.go

View check run for this annotation

Codecov / codecov/patch

pkg/app/piped/eventwatcher/eventwatcher.go#L661 

Added line #L661 was not covered by tests
}

args := argsTemplate{
Expand Down
Loading