Fix to register multiple events during a single fetch interval #2144
Conversation
|
Code coverage for golang is
|
| w.logger.Info(fmt.Sprintf("event watcher will update %d outdated values", len(commits))) | ||
| for _, c := range commits { | ||
| if err := tmpRepo.CommitChanges(ctx, tmpRepo.GetClonedBranch(), c.message, false, c.changes); err != nil { | ||
| if err := repo.CommitChanges(ctx, repo.GetClonedBranch(), c.message, false, c.changes); err != nil { |
There was a problem hiding this comment.
The root cause is that it tries to edit files before committing even though we've already written to files in L254.
So I settled on using tmpRepo only for local editing.
| func (w *watcher) updateValues(ctx context.Context, repo git.Repo, events []config.EventWatcherEvent, commitMsg string) error { | ||
| // Copy the repo to another directory to avoid pull failure in the future. | ||
| // Copy the repo to another directory to modify local file to avoid reverting previous changes. | ||
| tmpDir, err := ioutil.TempDir("", "event-watcher") |
There was a problem hiding this comment.
Previously, to avoid pull failure in the future, it copied the repository.
Ref: #1411 (comment)
However, we should assume that the commit history of the repository will be broken for some reason kind of like the possibility of force pushes and so on.
|
Code coverage for golang is
|
|
Nice catch |
| // from the control-plane to compare the value with one in the git repository. | ||
| func (w *watcher) Run(ctx context.Context) error { | ||
| w.logger.Info("start running event watcher") | ||
|
|
There was a problem hiding this comment.
How about making a temporary working directory for watcher and storing all cloned source code into that directory?
By that way, we can add a defer to clean the whole directory to ensure that everything will be removed when the watcher stopped.
And don't have to worry about this ignore:
repo, _ = w.cloneRepo(ctx, repoCfg)
|
Code coverage for golang is
|
|
Applied the working directory solution so that it can remove all repository at last. (@nghialv 's idea!) I settled on committing right after writing all changes in an event to a file. It is no problem to call writeFile twice (changing |
|
Code coverage for golang is
|
| w.logger.Info("Try to re-clone because it's more likely to be unable to pull the next time too", | ||
| zap.String("repo-id", repoCfg.RepoID), | ||
| ) | ||
| repo, _ = w.cloneRepo(ctx, repoCfg) |
There was a problem hiding this comment.
Is it ok to ignore the retuned error?
There was a problem hiding this comment.
All we have to do here is just emit error log. cloneRepo() does it, that's why it ignores the error.
There was a problem hiding this comment.
But conventionally, cloneRepo() should just give back an error and caller emits log.
|
|
||
| // cloneRepo clones the git repository under the working directory. | ||
| func (w *watcher) cloneRepo(ctx context.Context, repoCfg config.PipedRepository) (git.Repo, error) { | ||
| repo, err := w.gitClient.Clone(ctx, repoCfg.RepoID, repoCfg.Remote, repoCfg.Branch, filepath.Join(w.workingDir, repoCfg.RepoID)) |
There was a problem hiding this comment.
Maybe we should use a random directory instead of fixing it with the repository ID because this could be affected by the error at line 142.
| for _, e := range events { | ||
| latestEvent, ok := w.eventGetter.GetLatest(ctx, e.Name, e.Labels) | ||
| if !ok { | ||
| if err := w.commitFiles(ctx, &e, tmpRepo, commitMsg); err != nil { |
There was a problem hiding this comment.
&e is pointing to the same one. So we should passing struct instead of a pointer.
There was a problem hiding this comment.
It is. Actually it works well at this logic. But we'd prefer copying it.
|
Code coverage for golang is
|
|
Fixed |
|
Code coverage for golang is
|
|
Nice. |
|
🚀 |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR fixes two EventWatcher issues that:
Which issue(s) this PR fixes:
Fixes #1934
Does this PR introduce a user-facing change?: