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

Disable mans import task #1227

Merged
merged 7 commits into from
May 27, 2020
Merged

Disable mans import task #1227

merged 7 commits into from
May 27, 2020

Conversation

berggren
Copy link
Contributor

The recent addition of a .mans file importer raises a couple of concerns.

  1. The library is depending on ciso8601 which is a C library with a SWIG wrapper. This breaks our Docker builds and adds unnecessary dependencies.
  2. It does raw ES inserts, which means that we put a lot of trust into how the library deals with ES. It's a bit fragile, and I would like to be able to get a stream of events (either dict or Dataframes) and then use TS internal ES interface.

This PR disables the feature for now until we have sorted these things out with @garanews . I will raise a couple of issues in the mans_to_es project.

@berggren berggren requested a review from kiddinn May 26, 2020 16:02
@berggren
Copy link
Contributor Author

berggren commented May 26, 2020

Hey @garanews heads-up. I need to temporary disable this feature because it makes building our docker containers a bit too complex. I filed LDO-CERT/mans_to_es#10 - let me know what you think and I hope we can enable this importer soon again.

@berggren
Copy link
Contributor Author

@garanews I also added this FR for mans_to_es
LDO-CERT/mans_to_es#11

@kiddinn
Copy link
Contributor

kiddinn commented May 27, 2020

can you fix/resolve the conflicts and update the branch?

Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

can you fix the conflicts and update the branch?

Copy link
Contributor

@kiddinn kiddinn left a comment

Choose a reason for hiding this comment

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

LGTM

@berggren berggren merged commit 1325421 into master May 27, 2020
@berggren berggren deleted the disable-mans branch May 27, 2020 18:28
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.

3 participants