logstash-plugins / logstash-patterns-core

Apache License 2.0
2.17k stars 980 forks source link

Comma separated IP regex is handy for http-x-forwarded-for for instance #300

Closed leventyalcin closed 3 years ago

leventyalcin commented 3 years ago

Title is self-explanatory

cla-checker-service[bot] commented 3 years ago

❌ Author of the following commits did not sign a Contributor Agreement: d8cce587ee9ed2ecee4909f46b24997ecbd8c55b

Please, read and sign the above mentioned agreement if you want to contribute to this project

leventyalcin commented 3 years ago

I don't necessarily understand the reason for CLA though. Logstash itself is an Apache License software and I'm contributing to that. While Apache License 2 is allowing to anyone to use, change, distribute etc., why I should sign another agreement which is ~now~ not clear to me?

kares commented 3 years ago

Thanks for the contribution - not sure this is justified esp. since it's an addition to the base patterns. It isn't being re-used anywhere else and the potential of "comma-separated" things could easily explode.

These kinds of specific re-usable pieces are why we allow the filter grok to be extended with user patterns.

We also no longer accept anything wout proper test coverage as we did in the past.

leventyalcin commented 3 years ago

Thanks for the contribution - not sure this is justified esp. since it's an addition to the base patterns.

I think that should be in the contribution section on the README then. Also, README encourages any contribution and made me think justification will be on the PR. So, the process is confusing then.

It isn't being re-used anywhere else and the potential of "comma-separated" things could easily explode.

I'd rather this elaborated.

These kinds of specific re-usable pieces are why we allow the filter grok to be extended with user patterns.

The piece I sent could help more than QS as the value of QS appears with quotes included in ES. I'd like to get x-forwarded-for from combined log as 192.168.1.1, 192.168.2.2 instead "192.168.1.1, 192.168.2.2" Also, the pattern is widely used. Instead many of us rewrite this again and again, it would be nice to have it in the core.

We also no longer accept anything wout proper test coverage as we did in the past.

Fair enough.

As a result, the whole process is not clear and confusing. Scope of CLA is not clear and no explanation why it's needed. Also, the future of logstash is unknown to me (either will go under SSLP or stay as Apache License). So, I'm going to close this PR and I'll probably try to contribute OSS when a reliable initiative forked the project.

Thanks for taking your time and looking into this PR @kares .

kares commented 3 years ago

As a result, the whole process is not clear and confusing. Scope of CLA is not clear and no explanation why it's needed. Also, the future of logstash is unknown to me (either will go under SSLP or stay as Apache License).

Sorry about that, definitely might get confusing for some of the plugins - we're in quite legacy territory here and the team can only do so much with managing all of the plugins + LS itself. Having decent READMEs is really something we're behind on.

from ... Author of the following commits did not sign a Contributor Agreement :

The CLA is a license agreement, which enables Elastic to distribute your code without restriction. It doesn't require you to assign to us any copyright you have, the ownership of which remains in full with you. You cannot withdraw permission for its use at a later date.


Let me share my perspective I've spent the past months reviewing every pattern there is - in an effort to make them all ECS compliant while maintaining compatibility to ease migration.

There are patterns that are just there because someone contributed something "fairly usable" for their quick use-case - then years later we can not figure out what is it supposed to match (due no tests/examples) but we need to keep maintaining it.

Not saying this particular piece would be that hard to maintain but your self-explanatory title wasn't that explanatory to me :wink:

So, I'm going to close this PR and I'll probably try to contribute OSS when a reliable initiative forked the project.

:disappointed: