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 Data - CSV #7372

Merged
merged 391 commits into from
Jun 17, 2016
Merged

Add Data - CSV #7372

merged 391 commits into from
Jun 17, 2016

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Jun 3, 2016

Fixes #6541

This PR merges the long running feature/ingest branch into master. It contains code for both the Tail A File and CSV Add Data wizards, but only the CSV wizard is currently enabled. The CSV wizard is a simplified version of what's described in the ticket. It omits the pipeline building step because we want to implement editable pipelines before making that feature available.

This looks massive, but all of the code has been reviewed in separate PRs that targeted the feature/ingest branch. This PR mostly just needs eyes on the UI itself to ensure it's ready to merge into master.

BigFunger and others added 30 commits March 15, 2016 15:46
Updates the ingest POST endpoint to use Kibana processor schema
Implements a pattern checker directive for use in the Add Data UI
Fixing a few extra require statements hanging about in the ingest code
[es6ify] re-apply automated transforms after ingest work
@Bargs
Copy link
Contributor Author

Bargs commented Jun 14, 2016

jenkins, test it

@Bargs
Copy link
Contributor Author

Bargs commented Jun 14, 2016

jenkins, test it

@Bargs
Copy link
Contributor Author

Bargs commented Jun 14, 2016

@tylersmalley alright, no conflicts or test failures! Could you take another look?

@Bargs
Copy link
Contributor Author

Bargs commented Jun 14, 2016

@BigFunger I don't know how I could have left you out of this one :)

@BigFunger
Copy link
Contributor

BigFunger commented Jun 15, 2016

@BigFunger I don't know how I could have left you out of this one :)

@Bargs I can only assume it was through an act of kindness.

@tylersmalley
Copy link
Contributor

I uploaded over a dozen random CSV's from data.gov and didn't run into any issues. Obviously, the flow will be much improved when the ingest pipeline is merged in, but the messaging is good and allows you to manually correct issues and re-upload.

Functionally, this LGTM.

@Bargs
Copy link
Contributor Author

Bargs commented Jun 16, 2016

@tylersmalley @BigFunger I updated this PR to add the Upload CSV wizard to the new management screen. Could you guys take one more look? Assuming there are no issues this should be the last change required.

One note: some of the colors are a little off in the context of the new management screen, but I think it makes sense to handle this along with the clean up of management as a whole, rather than adding a bunch of super specific overrides to Add Data right now.

.nav-buttons {
float:right;
}
.nav-buttons {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure you want to define this style here? Before it was confined to the kbn-management-indices directive. Now it's applied to the entire application. Also, where is this class/style being used?

@BigFunger
Copy link
Contributor

Functionally LGTM, just had a small less/css question.

@@ -1,4 +1,4 @@
import PluginsKibanaSettingsSectionsIndicesRefreshKibanaIndexProvider from 'plugins/kibana/settings/sections/indices/_refresh_kibana_index';
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@tylersmalley
Copy link
Contributor

tylersmalley commented Jun 17, 2016

Clicking on the "Upload CSV", while already within /management/data/upload/* results in a blank page:

screenshot 2016-06-17 11 07 29

Appears that the currentStep is not being reset.

* Switched data/upload url to data/csv for better future proofing
* Removed old unnecessary css rule
* Fixed issue with management tab link removing the wizard's app state
@Bargs
Copy link
Contributor Author

Bargs commented Jun 17, 2016

@tylersmalley @BigFunger I addressed all of your feedback, please take another look.

@tylersmalley
Copy link
Contributor

As soon as this turns green, it LGTM! 🐑 🇮🇹

@epixa
Copy link
Contributor

epixa commented Jun 17, 2016

jenkins, test it

@BigFunger
Copy link
Contributor

LGTM!

@Bargs Bargs merged commit 15a4fa1 into master Jun 17, 2016
@epixa epixa deleted the feature/ingest branch December 15, 2016 18:26
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants