Change to spawn image watcher workers for each git repository #1270

nakabonne · 2020-12-17T05:30:46Z

What this PR does / why we need it:
This PR makes the worker's unit git repository. Due to this, it changes to use another directory to touch the image.

Which issue(s) this PR fixes:

Fixes #1261
Fixes #1262
Fixes #1263

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2020-12-17T05:36:59Z

Code coverage for golang is 34.28%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/config/piped.go	PipedImageProvider.UnmarshalJSON	`85.71%`	`88.89%`	`+3.17%`
pkg/config/piped.go	PipedImageWatcher.Validate	`100.00%`	`100.00%`	`+0.00%`

nakabonne · 2020-12-17T05:38:22Z

pkg/app/piped/imagewatcher/watcher.go

+		return fmt.Errorf("unknown image provider %s is defined", target.Provider)
 	}
-	cfg, ok, err := config.LoadImageWatcher(repo.GetPath(), includes, excludes)
+	provider, err := imageprovider.NewProvider(&providerCfg, w.logger)


Initially, I was thinking about making clients singletons but stopped doing so.
ECR client looks not to do write operation to itself. But the investigation for others hasn't done yet. So for their safety, I decided to create clients every time for now.

pkg/config/piped.go

nghialv · 2020-12-17T06:17:32Z

pkg/app/piped/imagewatcher/watcher.go

+
+	// Periodically update this cloned directory as long as this worker continues.
+	repo, err := w.gitClient.Clone(ctx, repoCfg.RepoID, repoCfg.Remote, repoCfg.Branch, "")
+	if err != nil {


When one of the goroutine stops due to this error, should we cancel all of the others?
Currently, this one is stopped and the other ones still running and just an error was logged.

Right, we should ensure all workers properly start running. Using sync/errgroup would be better.

pkg/app/piped/imagewatcher/watcher.go

nghialv · 2020-12-17T06:22:29Z

pkg/app/piped/imagewatcher/watcher.go

+		return fmt.Errorf("failed to create a new temporary directory: %w", err)
+	}
+	defer os.RemoveAll(tmpDir)
+	repo, err := w.gitClient.Clone(ctx, repoCfg.RepoID, repoCfg.Remote, repoCfg.Branch, tmpDir)


repo.Copy may be better. Because cloning is retrieving a new git tree. The commit-tree could be added from the last pull.

Yup, definitely right.

nghialv · 2020-12-17T06:23:54Z

pkg/app/piped/imagewatcher/watcher.go

+			for _, target := range cfg.Targets {
+				if err := w.updateOutdatedImage(ctx, &target, repoCfg, repo.GetPath(), commitMsg); err != nil {
+					w.logger.Error("failed to update image",
+						zap.String("repo-id", repoCfg.RepoID),


Instead of checking and updating for each target, how about checking and adding the commit for each target and then pushing once after all.

Thank you. That way can reduce the number of pushing to git. But I'm a bit worried about the commit will be unclear. Including all changes into a single commit may make it tough to revert

I know this is mainly for dev environment, but just warried.

No, we add multiple commits (each commit for each change) and push after all.

Ah, now I can understand. For that, we should share a temporary directory between all targets, but it looks like no problem to do.

Thank you! It looks the best way!

nakabonne · 2020-12-17T08:24:46Z

@nghialv Fixed them! PTAL 😄

pipecd-bot · 2020-12-17T08:27:14Z

Code coverage for golang is 34.28%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/config/piped.go	PipedImageProvider.UnmarshalJSON	`85.71%`	`88.89%`	`+3.17%`
pkg/config/piped.go	PipedImageWatcher.Validate	`100.00%`	`100.00%`	`+0.00%`

nghialv · 2020-12-17T08:44:50Z

pkg/app/piped/imagewatcher/watcher.go

-	}
-	cfg, ok, err := config.LoadImageWatcher(repo.GetPath(), includes, excludes)
+	defer os.RemoveAll(tmpDir)
+	tmpRepo, err := repo.Copy(tmpDir)


This Copy is done every 5 minutes even if there are no images to update.
I think it is a waste.
It would be better to check the outdated images based on the original read-only repo and then if there are any images to update, let copy to do.

Right, will fix it.

nakabonne · 2020-12-17T09:28:38Z

@nghialv Thank you for your polite review! Could you take a look again?

pipecd-bot · 2020-12-17T09:34:28Z

Code coverage for golang is 34.28%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/config/piped.go	PipedImageWatcher.Validate	`100.00%`	`100.00%`	`+0.00%`
pkg/config/piped.go	PipedImageProvider.UnmarshalJSON	`85.71%`	`88.89%`	`+3.17%`

nakabonne · 2020-12-17T09:56:10Z

/hold

nakabonne · 2020-12-17T09:55:56Z

pkg/app/piped/imagewatcher/watcher.go

-	if !ok {
-		return nil, fmt.Errorf("configuration file for Image Watcher not found: %w", err)
+	for _, c := range commits {
+		err := tmpRepo.CommitChanges(ctx, tmpRepo.GetClonedBranch(), commitMsg, false, map[string][]byte{


dumb, c.message should be used.

nakabonne · 2020-12-17T12:49:13Z

/hold cancel

pipecd-bot · 2020-12-17T12:54:44Z

Code coverage for golang is 34.28%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/config/piped.go	PipedImageProvider.UnmarshalJSON	`85.71%`	`88.89%`	`+3.17%`
pkg/config/piped.go	PipedImageWatcher.Validate	`100.00%`	`100.00%`	`+0.00%`

pkg/config/piped.go

pkg/app/piped/imagewatcher/watcher.go

nghialv · 2020-12-18T01:54:03Z

I have just left some suggestions. Please check again.

Co-authored-by: Le Van Nghia <[email protected]>

pipecd-bot · 2020-12-18T01:55:29Z

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

pipecd-bot · 2020-12-18T01:55:46Z

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

pipecd-bot · 2020-12-18T01:55:46Z

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

Co-authored-by: Le Van Nghia <[email protected]>

pipecd-bot · 2020-12-18T01:57:01Z

The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by /golinter trigger command right now.

You can check the build log from here.

pipecd-bot · 2020-12-18T01:57:44Z

The following files are not gofmt-ed. By commenting /golinter fmt, the formatted one will be appended to this pull request automatically.

pkg/app/piped/imagewatcher/watcher.go

--- pkg/app/piped/imagewatcher/watcher.go.orig
+++ pkg/app/piped/imagewatcher/watcher.go
@@ -36,7 +36,7 @@
 
 const (
 	defaultCommitMessageFormat = "Update image %s to %s defined at %s in %s"
-	defaultCheckInterval        = 5 * time.Minute
+	defaultCheckInterval       = 5 * time.Minute
 )
 
 type Watcher interface {
@@ -102,7 +102,7 @@
 	defer w.wg.Done()
 
 	var (
-		checkInterval               = defaultCheckInterval
+		checkInterval              = defaultCheckInterval
 		commitMsg                  string
 		includedCfgs, excludedCfgs []string
 	)

nakabonne · 2020-12-18T01:59:04Z

/golinter fmt

nakabonne · 2020-12-18T02:01:34Z

Fixed done!

nghialv · 2020-12-18T02:05:02Z

Nice.
/approve

pipecd-bot · 2020-12-18T02:05:13Z

Code coverage for golang is 34.28%. This pull request increases coverage by 0.00%.

File	Function	Base	Head	Diff
pkg/config/piped.go	PipedImageProvider.UnmarshalJSON	`85.71%`	`88.89%`	`+3.17%`
pkg/config/piped.go	PipedImageWatcher.Validate	`100.00%`	`100.00%`	`+0.00%`

nghialv · 2020-12-18T02:05:20Z

/approve cancel

nghialv · 2020-12-18T02:05:36Z

LOL
/lgtm

nakabonne · 2020-12-18T02:07:49Z

haha
/cc @khanhtc1202

khanhtc1202 · 2020-12-18T02:20:59Z

🚀
/approve

pipecd-bot · 2020-12-18T02:21:03Z

This pull request is APPROVED by khanhtc1202.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Change to spawn image watcher workers for each git repository

b5b2ab4

pipecd-bot added release-note-none size/L area/piped labels Dec 17, 2020

pipecd-bot requested review from khanhtc1202 and nghialv December 17, 2020 05:30

nakabonne commented Dec 17, 2020

View reviewed changes

nghialv reviewed Dec 17, 2020

View reviewed changes

Fix

50694bd

nghialv reviewed Dec 17, 2020

View reviewed changes

Copy repo when needed

df5f42a

pipecd-bot added the do-not-merge/hold label Dec 17, 2020

nakabonne commented Dec 17, 2020

View reviewed changes

Use commit message in each commit

ba99da5

pipecd-bot removed the do-not-merge/hold label Dec 17, 2020

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/config/piped.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/config/piped.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nghialv reviewed Dec 18, 2020

View reviewed changes

pkg/app/piped/imagewatcher/watcher.go Outdated Show resolved Hide resolved

nakabonne and others added 2 commits December 18, 2020 10:55

Update pkg/config/piped.go

4c7240a

Co-authored-by: Le Van Nghia <[email protected]>

Update pkg/config/piped.go

6d55fb0

Co-authored-by: Le Van Nghia <[email protected]>

Apply suggestions from code review

2063441

Co-authored-by: Le Van Nghia <[email protected]>

Format unfmt-ed files

6974072

pipecd-bot added the approved label Dec 18, 2020

pipecd-bot removed the approved label Dec 18, 2020

pipecd-bot added the lgtm label Dec 18, 2020

pipecd-bot added the approved label Dec 18, 2020

pipecd-bot merged commit 7e2e9aa into master Dec 18, 2020

pipecd-bot deleted the watch-repo branch December 18, 2020 02:21

pipecd-bot mentioned this pull request Dec 22, 2020

Fix malformed summary of the quick sync case #1316

Merged

Change to spawn image watcher workers for each git repository #1270

Change to spawn image watcher workers for each git repository #1270

Uh oh!

Conversation

nakabonne commented Dec 17, 2020

Uh oh!

pipecd-bot commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 17, 2020

Uh oh!

pipecd-bot commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 17, 2020

Uh oh!

pipecd-bot commented Dec 17, 2020

Uh oh!

nakabonne commented Dec 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne commented Dec 17, 2020

Uh oh!

pipecd-bot commented Dec 17, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nghialv commented Dec 18, 2020

Uh oh!

pipecd-bot commented Dec 18, 2020

Uh oh!

pipecd-bot commented Dec 18, 2020

Uh oh!

pipecd-bot commented Dec 18, 2020

Uh oh!

pipecd-bot commented Dec 18, 2020

Uh oh!

pipecd-bot commented Dec 18, 2020

Uh oh!

nakabonne commented Dec 18, 2020

Uh oh!

nakabonne commented Dec 18, 2020

Uh oh!

nghialv commented Dec 18, 2020

Uh oh!