Add configurations for Event watcher #1399

nakabonne · 2021-01-12T06:41:36Z

What this PR does / why we need it:
Most of the core logic is identical to ImageWatcher's one.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

pipecd-bot · 2021-01-12T06:47:43Z

Code coverage for golang is 34.13%. This pull request increases coverage by 0.33%.

File	Function	Base	Head	Diff
pkg/config/event_watcher.go	LoadEventWatcher	`--`	`72.73%`	`+72.73%`
pkg/config/event_watcher.go	filterEventWatcherFiles	`--`	`100.00%`	`+100.00%`
pkg/config/event_watcher.go	EventWatcherSpec.Validate	`--`	`87.50%`	`+87.50%`
pkg/config/piped.go	PipedEventWatcher.Validate	`--`	`0.00%`	`+0.00%`
pkg/config/analysis_template.go	AnalysisTemplateSpec.Validate	`0.00%`	`100.00%`	`+100.00%`
pkg/config/config.go	Config.init	`60.87%`	`80.00%`	`+19.13%`

nghialv · 2021-01-12T06:47:14Z

pkg/config/testdata/.pipe/event-watcher.yaml

+      replacements:
+        - file: app2/.pipe.yaml
+          yamlField: $.spec.input.helmChart.version
+      labels:


nit: Moving this labels to just above replacements.

nghialv · 2021-01-12T06:49:24Z

pkg/config/piped.go

 }
+
+type PipedEventWatcher struct {
+	// Interval to fetch the latest event and compare it with one defined in replacement files.


"defined in EventWatcher config files."

nghialv · 2021-01-12T06:52:25Z

pkg/config/piped.go

+
+// Validate checks if:
+// - empty repo ids exist
+// - duplicated repository settings exist


nit: The detailed comments can be moved inside the function, right next to its code.

// Validate the existence of repo ID. ... // Validate something else ...

nghialv · 2021-01-12T06:56:44Z

pkg/config/piped.go

+// - duplicated repository settings exist
+func (p *PipedEventWatcher) Validate() error {
+	repos := make(map[string]struct{}, len(p.GitRepos))
+	for i := 0; i < len(p.GitRepos); i++ {


nit:

for i, repo := range p.GitRepos { if repo.RepoID == "" { return fmt.Errorf("missing repoID at index %d"... } }

nghialv · 2021-01-12T06:58:35Z

pkg/config/event_watcher.go

+	Replacements []EventWatcherReplacement `json:"replacements"`
+	// Additional attributes of event. This can make an event definition
+	// unique even if the one with the same name exists.
+	Labels map[string]string `json:"labels"`


nit: Moving this to just after the Name?
I mean we have 2 parts: conditions for matching: name and labels and actions: replacements

I don't exactly get what you said. Sorry but could you show me some examples of that?

I want to say that our configuration will have 2 groups:

first group is about how to match the event. This contains "name" and "labels"

second group is about how to handle when the event matches. This contains "replacements"

So I think we should change the order to group them for more readable.

That makes sense! I placed the Label field at the end because this is optional, but putting after Name is more readable in such case.

nghialv · 2021-01-12T07:00:06Z

pkg/config/event_watcher.go

+
+// EventWatcherEvent defines which file will be replaced when the given event happened.
+type EventWatcherEvent struct {
+	// The event name. The combination of this name and labels must be unique within a project.


Do we need this statement: "The combination of this name and labels must be unique within a project."
Because piped is only handling the events of its project.

Sorry for the confusion, as you feel this shouldn't be here.

I just mean that "if you use the same name across repositories, they affect each other. So you have to make that combination unique if you want to explicitly distinguish them".

nghialv · 2021-01-12T07:00:38Z

pkg/config/event_watcher.go

+type EventWatcherEvent struct {
+	// The event name. The combination of this name and labels must be unique within a project.
+	Name string `json:"name"`
+	// List of places where will be replaced when the new event found.


nit: "new event matches"

nghialv · 2021-01-12T07:05:25Z

pkg/config/event_watcher.go

+
+func (s *EventWatcherSpec) Validate() error {
+	if len(s.Events) == 0 {
+		return ErrNotFound


Events are required?

I thought so because it looks wierd that no events despite the EventWatcher config file exists.

But on second thought, I realized it's not a problem to give back an EventWatcherSpec with no Events 👍

pipecd-bot · 2021-01-12T07:58:29Z

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. Support JSONField to replace values in json format

https://github.com/pipe-cd/pipe/blob/bca01b2b92f52f91dae5161934642b3306a9bff4/pkg/config/event_watcher.go#L45-L48

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.

2. Support HCLField to replace values in HCL format

https://github.com/pipe-cd/pipe/blob/bca01b2b92f52f91dae5161934642b3306a9bff4/pkg/config/event_watcher.go#L46-L49

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.

3. Consider merging events if there are events whose combination of name and labels is the same

https://github.com/pipe-cd/pipe/blob/bca01b2b92f52f91dae5161934642b3306a9bff4/pkg/config/event_watcher.go#L133-L136

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.

nakabonne · 2021-01-12T07:59:02Z

@nghialv Thank you for your polite review. Fixed them.

pipecd-bot · 2021-01-12T08:04:58Z

Code coverage for golang is 34.13%. This pull request increases coverage by 0.32%.

File	Function	Base	Head	Diff
pkg/config/event_watcher.go	LoadEventWatcher	`--`	`72.73%`	`+72.73%`
pkg/config/event_watcher.go	filterEventWatcherFiles	`--`	`100.00%`	`+100.00%`
pkg/config/event_watcher.go	EventWatcherSpec.Validate	`--`	`100.00%`	`+100.00%`
pkg/config/piped.go	PipedEventWatcher.Validate	`--`	`0.00%`	`+0.00%`
pkg/config/config.go	Config.init	`60.87%`	`80.00%`	`+19.13%`
pkg/config/analysis_template.go	AnalysisTemplateSpec.Validate	`0.00%`	`100.00%`	`+100.00%`

nghialv · 2021-01-12T08:16:30Z

Nice.
/lgtm

khanhtc1202 · 2021-01-12T08:27:07Z

🚀
/approve

pipecd-bot · 2021-01-12T08:27:10Z

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.

Add configurations for Event watcher

10e2ec5

pipecd-bot added release-note-none size/L labels Jan 12, 2021

pipecd-bot requested review from khanhtc1202 and nghialv January 12, 2021 06:41

nghialv reviewed Jan 12, 2021

View reviewed changes

Fix

bca01b2

pipecd-bot added the lgtm label Jan 12, 2021

pipecd-bot added the approved label Jan 12, 2021

pipecd-bot merged commit c137f9e into master Jan 12, 2021

pipecd-bot deleted the eventwatcher-config branch January 12, 2021 08:33

This was referenced Jan 12, 2021

Support JSONField to replace values in json format #1404

Closed

Support HCLField to replace values in HCL format #1405

Closed

Consider merging events if there are events whose combination of name and labels is the same #1406

Closed

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

Add configurations for Event watcher #1399

Add configurations for Event watcher #1399

Uh oh!

Conversation

nakabonne commented Jan 12, 2021

Uh oh!

pipecd-bot commented Jan 12, 2021

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 12, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Jan 12, 2021

1. Support JSONField to replace values in json format

This was created by todo plugin since "TODO:" was found in bca01b2 when #1399 was merged. cc: @nakabonne.

2. Support HCLField to replace values in HCL format

This was created by todo plugin since "TODO:" was found in bca01b2 when #1399 was merged. cc: @nakabonne.

3. Consider merging events if there are events whose combination of name and labels is the same

This was created by todo plugin since "TODO:" was found in bca01b2 when #1399 was merged. cc: @nakabonne.

Uh oh!

nakabonne commented Jan 12, 2021

Uh oh!

pipecd-bot commented Jan 12, 2021

Uh oh!

nghialv commented Jan 12, 2021

Uh oh!

khanhtc1202 commented Jan 12, 2021

Uh oh!

pipecd-bot commented Jan 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nakabonne Jan 12, 2021 •

edited

Loading

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.

This was created by todo plugin since "TODO:" was found in `bca01b2` when #1399 was merged. cc: @nakabonne.