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

[Pipeline Viewer] [Monitoring] Rename config view to PipelineViewer #20230

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Jun 26, 2018

Changes

The goal here is to rename the Config Viewer component to Pipeline Viewer. We've decided to do this to help keep consistency for users and ourselves; the previous component has been removed for the foreseeable future, so we are wanting to not add a point of confusion. This component was named "Config Viewer" originally to distinguish it from the existing component, but we no longer need that distinction.

From a high level, the config_view components were moved out of their own directory and into the home folder for PipelineViewer, where the old PV components used to live. Exports have been updated to mirror the way the old component was accessed.

Resolves #20123

Testing This PR

There aren't any functional changes in this PR, so a simple smoke test and green CI should be sufficient.

  1. Start up a logstash pipeline with at least one if...else branch, with X-Pack Monitoring enabled.
  2. Open the monitoring view, access your pipeline.
  3. Ensure that:
    a. Pipeline Viewer loads
    b. You are able to use Detail Drawer
    c. You can collapse/expand branches

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -30,8 +30,8 @@ uiModule.directive('monitoringLogstashPipelineViewer', ($injector) => {

scope.$watch('pipeline', (updatedPipeline) => {
pipelineState.update(updatedPipeline);
const configViewer = (
<ConfigViewer
const pipelineViewer = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend renaming this, or even better, it needn't be a variable, just put it into the render argument directly. too many bugs are caused by having 2 things only differentiated by casing.

This is just a recommendation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I'm fine with that. I'll ping you with an update commit.

@justinkambic
Copy link
Contributor Author

@mattapperson good for your 👀 94ceb41

@mattapperson
Copy link
Contributor

LGTM

@justinkambic
Copy link
Contributor Author

@mattapperson sorry to make you review this PR again.. but after running prettier on the directive file, this is what it gave us be8ae34

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM

@justinkambic justinkambic merged commit 068c02f into elastic:master Jun 26, 2018
justinkambic added a commit that referenced this pull request Jun 26, 2018
…20230)

* Rename config view to PipelineViewer.

* Remove unused className.

* Remove unneeded variable.

* Format file with prettier.
@justinkambic
Copy link
Contributor Author

Backported to:
6.4.0/6.x 794caae

@justinkambic justinkambic deleted the monitoring/rename-config-view-to-pipeline-viewer branch June 26, 2018 23:02
mattapperson pushed a commit that referenced this pull request Jun 28, 2018
…20230)

* Rename config view to PipelineViewer.

* Remove unused className.

* Remove unneeded variable.

* Format file with prettier.
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