Skip to content
This repository has been archived by the owner on Mar 21, 2023. It is now read-only.

Fix issue #173 #174

Closed
wants to merge 4 commits into from
Closed

Fix issue #173 #174

wants to merge 4 commits into from

Conversation

gianluca-valentini
Copy link

This is the fix for the above issue.
I did some unit test, please perform some end2end test before merging

@CLAassistant
Copy link

CLAassistant commented Apr 12, 2017

CLA assistant check
All committers have signed the CLA.

@joschi
Copy link
Contributor

joschi commented Apr 12, 2017

@gianluca-valentini Thanks for your contribution! We'll triage and take a look at it as soon as possible!

Please make sure that all existing unit tests pass your changes and that you keep using the code style of the existing code (indentation, order of imports etc.) to make reviewing this as swift as possible.

@joschi
Copy link
Contributor

joschi commented Apr 14, 2017

@andrea-tomassi Please don't commit IDE configuration files.

Additionally, if you intend to contribute to this PR, you also need to sign the CLA.

This reverts commit 6ab70da.
@andrea-tomassi
Copy link

@joschi sure, I already reverted this commit. It was just a mistake.

@gianluca-valentini
Copy link
Author

Hi guys,
can you tell me when the issue will be fixed or my pull request could be merged?

@joschi
Copy link
Contributor

joschi commented Apr 26, 2017

@gianluca-valentini We will review this PR as soon as the tests run successfully (see https://travis-ci.org/Graylog2/graylog-plugin-pipeline-processor/builds/222069537) and unnecessary whitespace changes have been reverted.

@gianluca-valentini
Copy link
Author

Hi,
I think that this issue is present on 2.2.3 too

@joschi
Copy link
Contributor

joschi commented Jun 12, 2017

@gianluca-valentini Any news on fixing the aforementioned issues with this pull request?

@gianluca-valentini
Copy link
Author

Hi @joschi
I have no news about this issue. I thought the correction could be applied soon.
Currently, looking the master stream, org.graylog.plugins.pipelineprocessor.functions.strings.RegexMatch still contains the old (and with bug) code.

I just underlined, some days ago, that this problem is present as in the master as in the v2.2.3 too.

remove the final attribute to RegexMatchResult boolean matches.
Looking the build error it seems that the *final* attribut could create that issue, even if on my environment (Eclipse and java 8) I don't have this problem.
@joschi
Copy link
Contributor

joschi commented Jun 12, 2017

@gianluca-valentini The build for the master branch on Travis CI is green (https://travis-ci.org/Graylog2/graylog-plugin-pipeline-processor/branches), so it seems that the build fails/failed because of a change in this PR.

Additionally, please address the issue with the whitespace changes (tabs vs. spaces) in this PR.

@gianluca-valentini
Copy link
Author

Hi @joschi, I changed the java using the online editor as I cannot find my local git project as I reset my pc some days ago.

joschi pushed a commit that referenced this pull request Jul 5, 2017
The regex() function only returned a single match (similar to the Regex Extractor),
while some users require it to return all matches.

Fixes #173
Closes #174
@joschi joschi self-assigned this Jul 5, 2017
@bernd bernd added this to the 2.4.0 milestone Jul 18, 2017
@joschi joschi removed their assignment Aug 11, 2017
@bernd
Copy link
Member

bernd commented Sep 21, 2017

@joschi @gianluca-valentini How do we continue with this?

@joschi
Copy link
Contributor

joschi commented Sep 21, 2017

@bernd I would close this PR without merging as it breaks the behavior of the regex() function and it doesn't seem to be possible to come up with a sensible solution given the harsh restrictions of the rule language.

See #197 for a cleaned up (and ultimately closed) version of this PR.

@bernd
Copy link
Member

bernd commented Sep 21, 2017

Okay, I am closing this. Thanks for the feedback!

@bernd bernd closed this Sep 21, 2017
@gianluca-valentini
Copy link
Author

Hi @joschi ,please can you explain me what do you mean when you say about this fix that it

breaks the behavior of the regex() function

What is the expected behavior, in your opinion, if more then one match happens for a single group?
thanks
Gianluca

@joschi
Copy link
Contributor

joschi commented Sep 21, 2017

@gianluca-valentini Feel free to follow up on #197 and its discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants