Implement Event watcher #1411

nakabonne · 2021-01-13T02:54:55Z

What this PR does / why we need it:
Add Event watcher, a piped component that periodically fetches the latest Event from the eventstore to compare the value with one in the git repository.

Most core logic is borrowed from imagewatcher being already. And soon-to-be enables this as another PR.

Which issue(s) this PR fixes:

Ref #1381

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-01-13T02:54:57Z

The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use /todo skip command.

Details

1. Retrieve the data from the control-plane.

https://github.com/pipe-cd/pipe/blob/ad24180b4169308cb0e93ba22aed22bb374ad145/pkg/app/piped/eventwatcher/eventwatcher.go#L201-L204

This was created by todo plugin since "FIXME:" was found in `ad24180` when #1411 was merged. cc: @nakabonne.

pipecd-bot

Some issues were detected while linting go source files in your changes.

pkg/app/piped/eventwatcher/eventwatcher.go

pipecd-bot · 2021-01-13T03:01:13Z

Code coverage for golang is 34.00%. This pull request decreases coverage by -0.06%.

File	Function	Base	Head	Diff
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineClusterRoleBindingHealth	`--`	`0.00%`	`+0.00%`
pkg/app/piped/cloudprovider/kubernetes/state.go	determineResourceHealth	`0.00%`	`0.00%`	`+0.00%`

pipecd-bot · 2021-01-15T01:31:13Z

Code coverage for golang is 33.95%. This pull request does not change code coverage.

@nghialv

**What this PR does / why we need it**: This PR adds a new `apistore` for Events according to @nghialv 's [suggestion](#1410 (comment)). It allows us to reduce communication number by caching the latest events. This will be used by #1411 at first. **Which issue(s) this PR fixes**: Ref #1381 **Does this PR introduce a user-facing change?**:  ```release-note NONE ``` This PR was merged by Kapetanios.

nghialv · 2021-01-15T05:29:56Z

pkg/app/piped/eventwatcher/eventwatcher.go

+			continue
+		}
+		// Put it into the change list.
+		newYml, err := yamlprocessor.ReplaceValue(yml, r.YAMLField, latestEvent.Data)


In case the users configured multiple fields for the same file, will this override the old changes?

~~Yes, that commit will happen conflict and end with failure.~~

~~But I assumed it doesn't happen if the latest values are the same. It happens when only the latest values are different from each other.~~

Ah, I just got what you mean. No. it always override the original content, defined in Git.

All of my decisions are based on the rule that never makes a change that crosses lines, but you know what I mean?

Ah, I totally got what you're most worried about. Yes, you're right. That can revert previous changes. Let me re-think it.

I feel like we can simply solve it by editing a local file inside this loop.

Yes. That is what I was talking about.

haha, finally I caught up.

I feel like we can simply solve it by editing a local file inside this loop.

I'm now inspecting if there are some issues with this way.

Looks like we have no problem with it, so applied it. PTAL 😄

pipecd-bot · 2021-01-15T06:23:28Z

Code coverage for golang is 33.95%. This pull request does not change code coverage.

nghialv · 2021-01-15T06:25:51Z

pkg/app/piped/eventwatcher/eventwatcher.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to replace value at %s with %s: %w", r.YAMLField, latestEvent.Data, err)
+		}
+		if err := ioutil.WriteFile(path, newYml, os.ModePerm); err != nil {


I have just realized that we have to save it in memory instead of writing to the local path.
Because writing it back to the local path will cause an error while doing the next pulls ??
(The data inside the repo must be read-only.)

So at line 215, checking it in the changes map before loading it from the local path looks good.

You are right! using cache looks good. Alternatively, I think we can leverage the temp repo because we use it when pushing. We can use it when making changes as well
https://github.com/pipe-cd/pipe/pull/1411/files#diff-60908ffb7e37bfc3692d8c4cace4be91d53bccf9a549fb91ea30b70387abd9cfR191

(Totally I misunderstood that we're using tmpRepo when making changes.)

Hmm... using tmp repo makes the logic simple. But on the dark side, copying the directory happens every time no matter commits exists. After thinking about that, I'm beginning to think the way to check changes you suggested is more appropriate.

IMO, checking changes looks better.

Good. Let me do so.

Hold on, I found it is still likely to be overwritten when processing other events. The changes is reset for each event. So looks like it's safer to use tmpRepo.

https://github.com/pipe-cd/pipe/pull/1411/files#diff-60908ffb7e37bfc3692d8c4cace4be91d53bccf9a549fb91ea30b70387abd9cfR170-R171

Right. lol. So let's update the local data. And please change the function name to match its new behavior.

Got it. The current one is absolutely not appropriate.

nakabonne · 2021-01-15T08:58:46Z

Thanks to deeper discussion, maybe we have the best one. Could you take a look?

pipecd-bot · 2021-01-15T09:01:14Z

Code coverage for golang is 33.95%. This pull request does not change code coverage.

nghialv · 2021-01-15T09:11:27Z

Nice.
/lgtm

nghialv · 2021-01-15T13:12:44Z

Let's merge this PR.
/approve

pipecd-bot · 2021-01-15T13:12:47Z

This pull request is APPROVED by nghialv.

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.

khanhtc1202 · 2021-01-15T13:20:30Z

ops, sorry I have not noticed this PR 🙇‍♂️

nghialv · 2021-01-15T13:22:21Z

@khanhtc1202 No problem. I just want to merge it for a nice weekend. lol.

khanhtc1202 · 2021-01-15T13:24:34Z

Have a nice weekend! 😂

nakabonne · 2021-01-15T13:33:15Z

Thank you both of you. It's going to make for a good weekend 😃

nghialv · 2021-01-15T13:35:59Z

Have a nice weekend. 🏃‍♂️

pipecd-bot added release-note-none do-not-merge/wip size/L area/piped labels Jan 13, 2021

pipecd-bot requested review from khanhtc1202 and nghialv January 13, 2021 02:54

pipecd-bot reviewed Jan 13, 2021

View reviewed changes

pkg/app/piped/eventwatcher/eventwatcher.go Outdated Show resolved Hide resolved

This was referenced Jan 13, 2021

Add Piped API to give back the latest Event #1410

Merged

Add apistore for caching the latest events #1429

Merged

Implement Event watcher

9a250c4

nakabonne force-pushed the event-watcher branch from ad24180 to 9a250c4 Compare January 15, 2021 01:25

nakabonne marked this pull request as ready for review January 15, 2021 05:10

pipecd-bot removed the do-not-merge/wip label Jan 15, 2021

nghialv reviewed Jan 15, 2021

View reviewed changes

Ensure to edit local file

4ed7886

nghialv reviewed Jan 15, 2021

View reviewed changes

Use tmp repository to modify files

8146908

pipecd-bot added the lgtm label Jan 15, 2021

pipecd-bot added the approved label Jan 15, 2021

pipecd-bot merged commit 4c66fbd into master Jan 15, 2021

pipecd-bot deleted the event-watcher branch January 15, 2021 13:19

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

nakabonne mentioned this pull request Jun 28, 2021

Fix to register multiple events during a single fetch interval #2144

Merged

Implement Event watcher #1411

Implement Event watcher #1411

Uh oh!

Conversation

nakabonne commented Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pipecd-bot commented Jan 13, 2021

1. Retrieve the data from the control-plane.

This was created by todo plugin since "FIXME:" was found in ad24180 when #1411 was merged. cc: @nakabonne.

Uh oh!

pipecd-bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pipecd-bot commented Jan 13, 2021

Uh oh!

pipecd-bot commented Jan 15, 2021

Uh oh!

nghialv Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakabonne Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 15, 2021

Uh oh!

nghialv Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jan 15, 2021

Uh oh!

pipecd-bot commented Jan 15, 2021

nakabonne commented Jan 13, 2021 •

edited

Loading

This was created by todo plugin since "FIXME:" was found in `ad24180` when #1411 was merged. cc: @nakabonne.

nghialv Jan 15, 2021 •

edited

Loading

nakabonne Jan 15, 2021 •

edited

Loading

nakabonne Jan 15, 2021 •

edited

Loading

nakabonne Jan 15, 2021 •

edited

Loading

nghialv Jan 15, 2021 •

edited

Loading

nghialv Jan 15, 2021 •

edited

Loading

nakabonne Jan 15, 2021 •

edited

Loading

nakabonne Jan 15, 2021 •

edited

Loading

khanhtc1202 commented Jan 15, 2021 •

edited

Loading