Add text regex support for Event watcher #2347

nakabonne · 2021-08-11T08:14:30Z

What this PR does / why we need it:
This PR adds the implementation of #2346 (comment)

Which issue(s) this PR fixes:

Fixes #2346
Fixes #2212
Fixes #2341
Fixes #2345

Does this PR introduce a user-facing change?:

Add text regex support for Event watcher

pipecd-bot · 2021-08-12T06:17:11Z

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 the "line" field that specifies tha line number staticaly

https://github.com/pipe-cd/pipe/blob/b7becc5309f989894c6cc1962d27c1093f28496d/pkg/config/event_watcher.go#L59-L62

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

pipecd-bot · 2021-08-12T06:24:45Z

Code coverage for golang is 31.99%. This pull request increases coverage by 0.07%.

File	Function	Base	Head	Diff
pkg/app/piped/eventwatcher/eventwatcher.go	modifyText	`--`	`76.47%`	`+76.47%`
pkg/config/event_watcher.go	EventWatcherReplacementTextField.Filled	`--`	`100.00%`	`+100.00%`
pkg/config/event_watcher.go	EventWatcherEvent.Validate	`73.68%`	`71.43%`	`-2.26%`
pkg/app/piped/eventwatcher/eventwatcher.go	watcher.commitFiles	`0.00%`	`0.00%`	`+0.00%`

nakabonne · 2021-08-13T07:54:31Z

We settled on employing #2346 (comment)
/hold

nakabonne · 2021-08-16T07:59:03Z

/hold cancel

pipecd-bot · 2021-08-16T08:05:43Z

Code coverage for golang is 32.08%. This pull request increases coverage by 0.16%.

File	Function	Base	Head	Diff
pkg/app/piped/eventwatcher/eventwatcher.go	modifyText	`--`	`86.67%`	`+86.67%`
pkg/app/piped/eventwatcher/eventwatcher.go	watcher.commitFiles	`0.00%`	`0.00%`	`+0.00%`
pkg/cache/memorycache/lru_cache.go	LRUCache.Get	`66.67%`	`100.00%`	`+33.33%`
pkg/config/event_watcher.go	EventWatcherEvent.Validate	`73.68%`	`71.43%`	`-2.26%`
pkg/regexpool/regexpool.go	Pool.Get	`87.50%`	`93.75%`	`+6.25%`

pipecd-bot · 2021-08-16T09:35:29Z

Code coverage for golang is 32.08%. This pull request increases coverage by 0.16%.

File	Function	Base	Head	Diff
pkg/app/piped/eventwatcher/eventwatcher.go	modifyText	`--`	`86.67%`	`+86.67%`
pkg/app/piped/eventwatcher/eventwatcher.go	watcher.commitFiles	`0.00%`	`0.00%`	`+0.00%`
pkg/cache/memorycache/lru_cache.go	LRUCache.Get	`66.67%`	`100.00%`	`+33.33%`
pkg/config/event_watcher.go	EventWatcherEvent.Validate	`73.68%`	`71.43%`	`-2.26%`
pkg/regexpool/regexpool.go	Pool.Get	`87.50%`	`93.75%`	`+6.25%`

pkg/config/event_watcher.go

nghialv · 2021-08-16T09:26:24Z

pkg/app/piped/eventwatcher/eventwatcher.go

 	return
 }
+
+func modifyText(path, regexText, newValue string) ([]byte, bool, error) {


Could you add a comment to show the meaning of each returned value?
Or using named return for better understand.

That's more helpful 👍

nghialv · 2021-08-16T09:33:21Z

pkg/app/piped/eventwatcher/eventwatcher_test.go

+		{
+			name:         "the file is up-to-date",
+			path:         "testdata/invalid.yaml",
+			regex:        "image: gcr.io/pipecd/foo:(v[0-9].[0-9].[0-9])",


Could you add a case that captures/replaces the whole tag url?

khanhtc1202 · 2021-08-16T10:01:55Z

pkg/app/piped/eventwatcher/eventwatcher.go

 	}
 	return
 }
+


nits, should we add a comment for this just as modifyYAML function has? It's a nits and it's up to you 🙆‍♀️

Co-authored-by: Khanh Tran <[email protected]>

pipecd-bot · 2021-08-17T01:23:14Z

Code coverage for golang is 32.08%. This pull request increases coverage by 0.16%.

File	Function	Base	Head	Diff
pkg/app/piped/eventwatcher/eventwatcher.go	modifyText	`--`	`86.67%`	`+86.67%`
pkg/config/event_watcher.go	EventWatcherEvent.Validate	`73.68%`	`71.43%`	`-2.26%`
pkg/regexpool/regexpool.go	Pool.Get	`87.50%`	`93.75%`	`+6.25%`
pkg/app/piped/eventwatcher/eventwatcher.go	watcher.commitFiles	`0.00%`	`0.00%`	`+0.00%`
pkg/cache/memorycache/lru_cache.go	LRUCache.Get	`66.67%`	`100.00%`	`+33.33%`

nakabonne · 2021-08-17T01:49:00Z

Applied, PTAL 😄

pipecd-bot · 2021-08-17T01:52:28Z

Code coverage for golang is 32.08%. This pull request increases coverage by 0.16%.

File	Function	Base	Head	Diff
pkg/app/piped/eventwatcher/eventwatcher.go	modifyText	`--`	`86.67%`	`+86.67%`
pkg/app/piped/eventwatcher/eventwatcher.go	watcher.commitFiles	`0.00%`	`0.00%`	`+0.00%`
pkg/cache/memorycache/lru_cache.go	LRUCache.Get	`66.67%`	`100.00%`	`+33.33%`
pkg/config/event_watcher.go	EventWatcherEvent.Validate	`73.68%`	`71.43%`	`-2.26%`
pkg/regexpool/regexpool.go	Pool.Get	`87.50%`	`93.75%`	`+6.25%`

nakabonne · 2021-08-17T02:01:15Z

I'm looking to add a couple of examples after this got mereged.

nghialv · 2021-08-17T02:03:08Z

Nice work!
/lgtm

pipecd-bot · 2021-08-17T02:41:47Z

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.

pipecd-bot added release-note-none area/piped do-not-merge/wip size/M labels Aug 11, 2021

pipecd-bot requested review from khanhtc1202 and nghialv August 11, 2021 08:14

Add text regex support for Event watcher

2812689

nakabonne force-pushed the text-regex-event-watcher branch from 0f0772c to 2812689 Compare August 12, 2021 06:13

pipecd-bot added size/L and removed size/M labels Aug 12, 2021

Add field comment

b7becc5

nakabonne marked this pull request as ready for review August 12, 2021 06:17

pipecd-bot removed the do-not-merge/wip label Aug 12, 2021

nakabonne mentioned this pull request Aug 12, 2021

Add text regex support for Event watcher #2346

Closed

pipecd-bot added the do-not-merge/hold label Aug 13, 2021

Use capturing group

07dd0b1

pipecd-bot added area/docs area/site labels Aug 16, 2021

pipecd-bot removed the do-not-merge/hold label Aug 16, 2021

pipecd-bot removed the release-note-none label Aug 16, 2021

Wrap errors

c6213a8

khanhtc1202 reviewed Aug 16, 2021

View reviewed changes

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

nghialv reviewed Aug 16, 2021

View reviewed changes

khanhtc1202 reviewed Aug 16, 2021

View reviewed changes

Update pkg/config/event_watcher.go

18318f5

Co-authored-by: Khanh Tran <[email protected]>

Fix

7441e9b

pipecd-bot added the lgtm label Aug 17, 2021

khanhtc1202 approved these changes Aug 17, 2021

View reviewed changes

pipecd-bot added the approved label Aug 17, 2021

pipecd-bot merged commit 200c623 into master Aug 17, 2021

pipecd-bot deleted the text-regex-event-watcher branch August 17, 2021 02:50

pipecd-bot mentioned this pull request Aug 19, 2021

Add AllowStrayAsViewer option #2370

Merged

Add text regex support for Event watcher #2347

Add text regex support for Event watcher #2347

Uh oh!

Conversation

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

Uh oh!

Uh oh!

pipecd-bot commented Aug 12, 2021

1. Support the "line" field that specifies tha line number staticaly

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

Uh oh!

pipecd-bot commented Aug 12, 2021

Uh oh!

nakabonne commented Aug 13, 2021

Uh oh!

nakabonne commented Aug 16, 2021

Uh oh!

pipecd-bot commented Aug 16, 2021

Uh oh!

pipecd-bot commented Aug 16, 2021

Uh oh!

Uh oh!

nghialv Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

nakabonne Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

nghialv Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

khanhtc1202 Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

pipecd-bot commented Aug 17, 2021

Uh oh!

nakabonne commented Aug 17, 2021

Uh oh!

pipecd-bot commented Aug 17, 2021

Uh oh!

nakabonne commented Aug 17, 2021

Uh oh!

nghialv commented Aug 17, 2021

Uh oh!

pipecd-bot commented Aug 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nakabonne commented Aug 11, 2021 •

edited

Loading

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