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

Filebeat wizard #5790

Merged
merged 29 commits into from
Feb 2, 2016
Merged

Filebeat wizard #5790

merged 29 commits into from
Feb 2, 2016

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Dec 30, 2015

Creates the shell for the tail a file wizard described in #5974

Requires #5956 which has been merged into this branch, so reviewers can ignore the files that were changed in that PR as they're being reviewed separately.

This PR only addresses the functionality of the wizard container, not the individual steps. It demonstrates how data will flow from step to step with placeholder directives for each step, and it provides the logic necessary for controlling the user's navigation (preventing the user from jumping to a step they're not ready for, warning them if they're going to navigate back and lose changes, and using app state to make the back-forward buttons work).

Styling will be addressed in a future PR once the design is finalized.

Original comment

@BigFunger I started working on the wizard that the individual ingest steps we're working on will live in. I wanted to get it up in a PR so you could see what direction I'm headed in and let me know what you think. Conceptually I'm thinking the individual steps (pipeline building, index pattern creation, etc) will be reusable directives that each optionally take some input and when they're finished, return some output to the wizard controller. The directives themselves shouldn't know or care that they're a part of a larger multi-step wizard, they just take some input and return some results while the wizard maintains state and ordering of the steps. That way each step can be reused across different wizards (filebeat, topbeat, csv, etc).

The wizard controller itself can probably be more general than what I have here, or even moved inside a directive, but I haven't quite figured that out yet.

(cherry picked from commit 8cbf211)
(cherry picked from commit 4975f57)
(cherry picked from commit 599e012)
(cherry picked from commit ad25d24)
var template = require('plugins/kibana/settings/sections/data/directives/install_filebeat_step.html');

modules.get('apps/settings')
.directive('installFilebeatStep', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Who needs eslint when I've got spalinter?

Copy link
Contributor

Choose a reason for hiding this comment

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

:P

@Bargs Bargs mentioned this pull request Jan 21, 2016
15 tasks
@Bargs Bargs added the review label Jan 23, 2016
@Bargs Bargs assigned spalger and unassigned BigFunger Jan 23, 2016
@Bargs
Copy link
Contributor Author

Bargs commented Jan 28, 2016

@spalger any chance you'll get to this soon? This PR blocks a number of other PRs that depend on it. If not perhaps @jbudz has some spare time to look at it?

@jbudz
Copy link
Member

jbudz commented Jan 28, 2016

Clicking 'don't show this alert again' the previous button/window.back doesn't work

@jbudz
Copy link
Member

jbudz commented Jan 28, 2016

Would it be possible to make the alert only show up if there are changes?

<a href="#/settings/indices/create/filebeat">Tail a File</a>
</h4>
<div>
Pick this option if you have log data you'd like to send to Elasticsearch.
Copy link
Member

Choose a reason for hiding this comment

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

thoughts on this saying log files or log file data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Log file data sounds ok to me. I wouldn't worry too much about the descriptive text, I'm guessing some wordsmith will refine these later on.

@spalger spalger assigned Bargs and unassigned spalger Feb 1, 2016
@Bargs
Copy link
Contributor Author

Bargs commented Feb 2, 2016

@jbudz You said "Clicking 'don't show this alert again' the previous button/window.back doesn't work". What browser are you using that gives you that option? I don't see that anywhere in Firefox, Safari, or Chrome.

@jbudz
Copy link
Member

jbudz commented Feb 2, 2016

Chrome 48. The first time the message shows up it won't be there, but cancelling once will cause it to show
image

@Bargs
Copy link
Contributor Author

Bargs commented Feb 2, 2016

Ah interesting.... do you know how we've handled this in other areas of the code that use this safe confirm utility?

@jbudz
Copy link
Member

jbudz commented Feb 2, 2016

I tested overwriting an index pattern, it causes the button to do nothing.

@Bargs
Copy link
Contributor Author

Bargs commented Feb 2, 2016

@jbudz It seems that checking that box is the equivalent of automatically clicking cancel for every alert. I don't see any way for the JS to tell the difference between that and a manual cancel either :(

I think this is something that will need a global solution, as discussed in #6003

@Bargs Bargs assigned spalger and unassigned Bargs Feb 2, 2016
@spalger
Copy link
Contributor

spalger commented Feb 2, 2016

LGTM, assuming build failures are simply caused by not being merged with master

@spalger spalger assigned Bargs and unassigned spalger Feb 2, 2016
Bargs pushed a commit that referenced this pull request Feb 2, 2016
@Bargs Bargs merged commit 64328a3 into elastic:feature/ingest Feb 2, 2016
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