Skip to content
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

Allow import/export of grok patterns in content packs #1300

Merged
merged 5 commits into from
Jul 16, 2015

Conversation

joschi
Copy link
Contributor

@joschi joschi commented Jul 13, 2015

Previously it wasn't possible to export or import grok patterns as part of a content pack. This could lead to broken extractors in inputs.

This PR adds the capabilities to import/export grok patterns in content packs.

.add("id", id)
.add("name='", name)
.add("pattern='", pattern)
.add("contentPack", contentPack)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tiny thing, but we should remove those =' from name and pattern, as MoreObjects.toStringHelper will take care of that

@edmundoa
Copy link
Contributor

There are a couple of issues I found:

  • Importing a content pack with grok patterns ignores them. I think the changes are missing the grokPatterns list in org/graylog2/restclient/models/api/requests/CreateBundleRequest.java.
  • We don't establish dependencies between extractors using grok patterns and grok patterns, so it is possible at the moment to export an extractor that uses a grok pattern, but not that pattern. Should we resolve those dependencies?

@joschi
Copy link
Contributor Author

joschi commented Jul 15, 2015

We don't establish dependencies between extractors using grok patterns and grok patterns, so it is possible at the moment to export an extractor that uses a grok pattern, but not that pattern. Should we resolve those dependencies?

I don't think that this is necessary now. In order to properly handle those dependencies, we'd need to introduce a (very) special handling for the GrokExtractor and analyze all used patterns recursively (as grok patterns can include other grok patterns) and then export all those required patterns. If people start complaining, we can still implement it, but for now I wouldn't want to put in those extra hours for this PR.

@edmundoa
Copy link
Contributor

Another alternative would be to export all grok patterns, but it's also not an optimal solution. Anyway, I think this is good enough and much better than what we had before!

LGTM after the last changes :shipit:

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

Successfully merging this pull request may close these issues.

2 participants