-
Notifications
You must be signed in to change notification settings - Fork 3.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: Replace stage in pipeline #2060
Conversation
1b55961
to
d676c3d
Compare
Codecov Report
@@ Coverage Diff @@
## master #2060 +/- ##
==========================================
- Coverage 61.31% 61.30% -0.01%
==========================================
Files 146 147 +1
Lines 11189 11316 +127
==========================================
+ Hits 6860 6937 +77
- Misses 3784 3816 +32
- Partials 545 563 +18
|
d676c3d
to
8774c11
Compare
bc11d7b
to
f37f68b
Compare
f37f68b
to
4c3f806
Compare
4c3f806
to
08698c2
Compare
Codecov Report
@@ Coverage Diff @@
## master #2060 +/- ##
==========================================
- Coverage 61.31% 61.25% -0.06%
==========================================
Files 146 147 +1
Lines 11189 11295 +106
==========================================
+ Hits 6860 6919 +59
- Misses 3784 3815 +31
- Partials 545 561 +16
|
7ee362e
to
8678482
Compare
afc3fcf
to
c342759
Compare
c342759
to
4eee2b1
Compare
} | ||
|
||
// All the named captured group will be extracted | ||
for i, name := range r.expression.SubexpNames() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is useful ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of you code would be much simpler.
Let's drop this for now. You can have a regex to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's drop this for now. You can have a regex to do that
This is just for reducing using different stages in pipeline. If user wants to extract variable and then template the extracted variable he should use regex stage and then template stage. This would reduce that effort
A lot of you code would be much simpler.
Sure. Let me know where I have to improve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is there any ETA when will this feature be released? Latest 1.5.0 seems to be released on May 20 and this was merged on Jun 1. |
} | ||
|
||
if c.Replace == "" { | ||
return nil, errors.New(ErrEmptyReplaceStageConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, I know this PR is merged, but why is this validated? If, for example, I want to remove part of the log, I cannot use replace pipeline as it won't accept an empty replace. I could use a whitespace, but then it looks funny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a fair request. You can open a PR or may be @adityacs is up for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll have some free time today or tomorrow to do a PR. Just wanted to ask your opinions about it :)
@slim-bean probably has an idea for the ETA. |
This week! |
What this PR does / why we need it:
Adds Replace stage in pipeline. Supports, replacing log lines based on captured groups.
Which issue(s) this PR fixes:
Fixes #2033
Checklist