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

Add Cisco integration 0.0.2 #370

Closed
wants to merge 0 commits into from
Closed

Add Cisco integration 0.0.2 #370

wants to merge 0 commits into from

Conversation

alakahakai
Copy link

Cisco integration 0.0.2

@alakahakai alakahakai requested review from ruflin and mtojek April 22, 2020 23:14
@@ -0,0 +1,15 @@
{{#if input == "syslog"}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is not correct in the handlebars syntax and needs to split in multiple stream files.

Unfortunately this is a known issue, but it requires some advanced templating processing - we didn't introduce it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As #346 was merged, we can start to use it in the packages but it will not be supported yet in the UI. I suggest we pick one of the two inputs for now.

@alakahakai Which one do you think is more important? syslog or file?

Copy link
Author

@alakahakai alakahakai Apr 23, 2020

Choose a reason for hiding this comment

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

syslog is more important, but I will add a separate file for file input. For handlebars, can we add the support for Ember Truth Helper addon?

@@ -0,0 +1,1281 @@
---
Copy link
Contributor

Choose a reason for hiding this comment

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

If this file contains any fields not mentioned in the fields files (e.g. event.code, event.provider), please create in Beats a file called fields.epr.yml and describe them there (there is a paragraph about this in CONTRIBUTING).

@mtojek
Copy link
Contributor

mtojek commented Apr 23, 2020

@alakahakai Thank you for the contribution. I left a couple of comments that can apply to few place in your contribution, but I didn't mark every single place (e.g. {{ if input == ... }}).

You can also extend the description of the issue with commands you use to import modules or test them manually.

Fingers crossed for serving this package through the registry and I'm happy to see some screenshots in the PR description with the UI.

@mtojek mtojek marked this pull request as draft April 23, 2020 05:49
@mtojek
Copy link
Contributor

mtojek commented Apr 23, 2020

Also, I converted your PR to draft to prevent an accidental merging. I used to convert them once I finish all necessary works (I mean, "in my opinion" the package is complete, it works in my environment and I see flowing data on dashboards).

@@ -0,0 +1,15 @@
{{#if input == "syslog"}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

As #346 was merged, we can start to use it in the packages but it will not be supported yet in the UI. I suggest we pick one of the two inputs for now.

@alakahakai Which one do you think is more important? syslog or file?

dev/packages/alpha/cisco-0.0.2/dataset/ftd/manifest.yml Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
TODO
Copy link
Collaborator

Choose a reason for hiding this comment

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

jep :-D

dev/packages/alpha/cisco-0.0.2/manifest.yml Outdated Show resolved Hide resolved
@mtojek
Copy link
Contributor

mtojek commented Apr 27, 2020

Could you please post some screenshots in the description?

- cisco-asa
- name: syslog_port
type: integer
title: Syslog Port
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a configuration option for Syslog Host or it's only port as it's localhost? Maybe a description would clarify this.

title: Tags
multi: true
required: true
show_user: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it required to show this field to user?

title: Log Level
multi: false
required: true
show_user: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: is it required to present it to user? I think screenshots will help us understand which fields are required.

required: true
show_user: true
default: 9002
- input: file
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you check if this option works? I wonder if this should be "file" or "log".

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I'm not convinced about showing all the options to the user. As I said, screenshots would be welcome here.

@alakahakai alakahakai closed this Apr 28, 2020
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.

4 participants