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

Ingest pipeline setup client #6497

Merged

Conversation

BigFunger
Copy link
Contributor

Replaces #6071 (2 of 3)
Creates the client-side functionality for node ingest simulate.

  • Adds the pipeline setup step to the ingest wizard
  • Adds the 'set' processor

@@ -0,0 +1,58 @@
import uiModules from 'ui/modules';
Copy link
Contributor

Choose a reason for hiding this comment

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

This file can be merged with the existing service in ui/public/ingest. Also when creating modules with Angular services it's preferable to just return the factory function so that the module can be used with the Private module loader.

Some tests for the simulatePipeline function would also be good.

return data;
}

return $http.post(`../api/kibana/ingest/simulate`, pack(pipeline))
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ingestAPIPrefix constant in the url

}

addExisting(existingProcessor) {
const Type = this._processorTypes[existingProcessor.typeId];
Copy link
Contributor

Choose a reason for hiding this comment

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

Did const Type = existingProcessor.constructor not work for some reason? I would have expected it to, and if it did this would be much simpler and eliminate the need for this _processorTypes property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted.

@Bargs
Copy link
Contributor

Bargs commented Mar 16, 2016

Left a few followups on existing inline comments and a few new ones. Overall this is looking great. Other than the inline comments, I think there are just a couple things left:

  • Update tests
  • Decide how to persist view state
  • If I get the API fixed quickly, we should be able to finish gluing this together with the pattern review step.
  • ??? Anything else you can think of?

@Bargs Bargs assigned BigFunger and unassigned Bargs Mar 16, 2016
@BigFunger
Copy link
Contributor Author

jenkins, test it

@BigFunger BigFunger assigned Bargs and unassigned BigFunger Mar 17, 2016
expect(actual).to.be(expected);
});

it('should throw an error if given an unknown id', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think throwing an error is the right thing to do here. Every usage of getProcessorById will need to be wrapped in a try/catch. I think it would be better to just return undefined.

@Bargs
Copy link
Contributor

Bargs commented Mar 17, 2016

Once the tests pass, this LGTM

BigFunger added a commit that referenced this pull request Mar 18, 2016
@BigFunger BigFunger merged commit ad349b5 into elastic:feature/ingest Mar 18, 2016
@Bargs
Copy link
Contributor

Bargs commented Mar 18, 2016

Whooo 🎉

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.

4 participants