Repository: aurora Updated Branches: refs/heads/master a30f936ae -> 0efe41572
Add diff viewer to Update Page Reviewed at https://reviews.apache.org/r/63083/ Project: http://git-wip-us.apache.org/repos/asf/aurora/repo Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/0efe4157 Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/0efe4157 Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/0efe4157 Branch: refs/heads/master Commit: 0efe41572cf3350e9afc9b84ab3e501516b26321 Parents: a30f936 Author: David McLaughlin <[email protected]> Authored: Tue Oct 17 14:57:27 2017 -0700 Committer: David McLaughlin <[email protected]> Committed: Tue Oct 17 14:57:27 2017 -0700 ---------------------------------------------------------------------- ui/src/main/js/components/ConfigDiff.js | 16 ++--- ui/src/main/js/components/Diff.js | 12 ++++ ui/src/main/js/components/InstanceViz.js | 4 +- ui/src/main/js/components/UpdateConfig.js | 5 ++ ui/src/main/js/components/UpdateDiff.js | 45 +++++++++++++ .../js/components/__tests__/ConfigDiff-test.js | 31 ++------- .../main/js/components/__tests__/Diff-test.js | 18 +++++ .../js/components/__tests__/UpdateDiff-test.js | 71 ++++++++++++++++++++ ui/src/main/js/test-utils/UpdateBuilders.js | 7 ++ ui/src/main/sass/app.scss | 1 + ui/src/main/sass/components/_diff.scss | 42 ++++++++++++ ui/src/main/sass/components/_job-page.scss | 43 ------------ ui/src/main/sass/components/_update-page.scss | 7 ++ 13 files changed, 224 insertions(+), 78 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/ConfigDiff.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/ConfigDiff.js b/ui/src/main/js/components/ConfigDiff.js index 9627751..9abcd68 100644 --- a/ui/src/main/js/components/ConfigDiff.js +++ b/ui/src/main/js/components/ConfigDiff.js @@ -1,5 +1,6 @@ import React from 'react'; -import { diffJson } from 'diff'; + +import Diff from 'components/Diff'; import { instanceRangeToString } from 'utils/Task'; @@ -52,17 +53,12 @@ export default class ConfigDiff extends React.Component { if (this.props.groups.length < 2) { return <div />; } - const result = diffJson( - this.props.groups[this.state.leftGroupIdx].config, - this.props.groups[this.state.rightGroupIdx].config); + return (<div className='task-diff'> {this.diffNavigation()} - <div className='diff-view'> - {result.map((r, i) => ( - <span className={r.added ? 'added' : r.removed ? 'removed' : 'same'} key={i}> - {r.value} - </span>))} - </div> + <Diff + left={this.props.groups[this.state.leftGroupIdx].config} + right={this.props.groups[this.state.rightGroupIdx].config} /> </div>); } } http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/Diff.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/Diff.js b/ui/src/main/js/components/Diff.js new file mode 100644 index 0000000..53ba517 --- /dev/null +++ b/ui/src/main/js/components/Diff.js @@ -0,0 +1,12 @@ +import React from 'react'; +import { diffJson } from 'diff'; + +export default function Diff({ left, right }) { + const result = diffJson(left, right); + return (<div className='diff-view'> + {result.map((r, i) => ( + <span className={r.added ? 'added' : r.removed ? 'removed' : 'same'} key={i}> + {r.value} + </span>))} + </div>); +} http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/InstanceViz.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/InstanceViz.js b/ui/src/main/js/components/InstanceViz.js index 99efec4..c8402e1 100644 --- a/ui/src/main/js/components/InstanceViz.js +++ b/ui/src/main/js/components/InstanceViz.js @@ -9,7 +9,9 @@ export default function InstanceViz({ instances, jobKey }) { return (<ul className={`instance-grid ${className}`}> {instances.map((i) => { - return (<Link key={i} to={`/beta/scheduler/${role}/${environment}/${name}/${i.instanceId}`}> + return (<Link + key={i.instanceId} + to={`/beta/scheduler/${role}/${environment}/${name}/${i.instanceId}`}> <li className={i.className} title={i.title} /> </Link>); })} http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/UpdateConfig.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/UpdateConfig.js b/ui/src/main/js/components/UpdateConfig.js index fac3c88..89dea25 100644 --- a/ui/src/main/js/components/UpdateConfig.js +++ b/ui/src/main/js/components/UpdateConfig.js @@ -2,8 +2,13 @@ import React from 'react'; import PanelGroup, { Container, StandardPanelTitle } from 'components/Layout'; import TaskConfig from 'components/TaskConfig'; +import UpdateDiff from 'components/UpdateDiff'; export default function UpdateConfig({ update }) { + if (update.update.instructions.initialState.length > 0) { + return <UpdateDiff update={update} />; + } + return (<Container> <PanelGroup noPadding title={<StandardPanelTitle title='Update Config' />}> <TaskConfig config={update.update.instructions.desiredState.task} /> http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/UpdateDiff.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/UpdateDiff.js b/ui/src/main/js/components/UpdateDiff.js new file mode 100644 index 0000000..c88ba85 --- /dev/null +++ b/ui/src/main/js/components/UpdateDiff.js @@ -0,0 +1,45 @@ +import React from 'react'; + +import Diff from 'components/Diff'; +import PanelGroup, { Container, StandardPanelTitle } from 'components/Layout'; + +import { instanceRangeToString } from 'utils/Task'; + +export default class UpdateDiff extends React.Component { + constructor(props) { + super(props); + this.state = {groupIdx: 0}; + } + + diffNavigation() { + const that = this; + const { initialState } = this.props.update.update.instructions; + const currentGroup = initialState[this.state.groupIdx]; + if (initialState.length < 2) { + return ''; + } else { + const otherOptions = initialState.map((g, i) => i).filter((i) => i !== that.state.groupIdx); + return (<div className='update-diff-picker'> + Current Instances: + <select onChange={(e) => this.setState({groupIdx: parseInt(e.target.value, 10)})}> + <option key='current'>{instanceRangeToString(currentGroup.instances)}</option> + {otherOptions.map((i) => (<option key={i} value={i}> + {instanceRangeToString(initialState[i].instances)} + </option>))} + </select> + </div>); + } + } + + render() { + const { initialState, desiredState } = this.props.update.update.instructions; + return (<Container> + <PanelGroup noPadding title={<StandardPanelTitle title='Update Diff' />}> + <div className='task-diff'> + {this.diffNavigation()} + <Diff left={initialState[this.state.groupIdx].task} right={desiredState.task} /> + </div> + </PanelGroup> + </Container>); + } +} http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/__tests__/ConfigDiff-test.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/__tests__/ConfigDiff-test.js b/ui/src/main/js/components/__tests__/ConfigDiff-test.js index 3eaa78a..8aaaf84 100644 --- a/ui/src/main/js/components/__tests__/ConfigDiff-test.js +++ b/ui/src/main/js/components/__tests__/ConfigDiff-test.js @@ -1,6 +1,7 @@ import React from 'react'; import { shallow } from 'enzyme'; +import Diff from '../Diff'; import ConfigDiff from '../ConfigDiff'; import { TaskConfigBuilder, createConfigGroup } from 'test-utils/TaskBuilders'; @@ -12,24 +13,14 @@ describe('ConfigDiff', () => { expect(el.contains(<div />)).toBe(true); }); - it('Should not add change classes to diff viewer when configs are same', () => { + it('Should default to showing a diff of the first two configs when multiple are provided', () => { const groups = [ createConfigGroup(TaskConfigBuilder, [0, 0]), - createConfigGroup(TaskConfigBuilder, [1, 9]) + createConfigGroup(TaskConfigBuilder, [1, 9]), + createConfigGroup(TaskConfigBuilder, [10, 20]) ]; const el = shallow(<ConfigDiff groups={groups} />); - expect(el.find('span.removed').length).toBe(0); - expect(el.find('span.added').length).toBe(0); - }); - - it('Should add change classes to diff viewer when configs are not the same', () => { - const groups = [ - createConfigGroup(TaskConfigBuilder, [0, 0]), - createConfigGroup(TaskConfigBuilder.tier('something-else'), [1, 9]) - ]; - const el = shallow(<ConfigDiff groups={groups} />); - expect(el.find('span.removed').length).toBe(1); - expect(el.find('span.added').length).toBe(1); + expect(el.contains(<Diff left={groups[0].config} right={groups[1].config} />)).toBe(true); }); it('Should not show any config group dropdown when there are only two groups', () => { @@ -59,17 +50,9 @@ describe('ConfigDiff', () => { createConfigGroup(TaskConfigBuilder.tier('something-else'), [2, 2]) ]; const el = shallow(<ConfigDiff groups={groups} />); - expect(el.find('span.removed').length).toBe(1); - expect(el.find('span.added').length).toBe(1); + expect(el.contains(<Diff left={groups[0].config} right={groups[1].config} />)).toBe(true); - expect(el.find('.diff-before select').length).toBe(1); - expect(el.find('option').length).toBe(4); - - // Change the left config to be index=2, which has the same config as index=1 el.find('.diff-before select').simulate('change', {target: {value: '2'}}); - // Now assert the diff was updated! - expect(el.find('span.removed').length).toBe(0); - expect(el.find('span.added').length).toBe(0); - expect(el.find('option').length).toBe(4); + expect(el.contains(<Diff left={groups[2].config} right={groups[1].config} />)).toBe(true); }); }); http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/__tests__/Diff-test.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/__tests__/Diff-test.js b/ui/src/main/js/components/__tests__/Diff-test.js new file mode 100644 index 0000000..7f91322 --- /dev/null +++ b/ui/src/main/js/components/__tests__/Diff-test.js @@ -0,0 +1,18 @@ +import React from 'react'; +import { shallow } from 'enzyme'; + +import Diff from '../Diff'; + +describe('Diff', () => { + it('Should not add change classes to diff viewer when objects are same', () => { + const el = shallow(<Diff left={{test: true}} right={{test: true}} />); + expect(el.find('span.removed').length).toBe(0); + expect(el.find('span.added').length).toBe(0); + }); + + it('Should add change classes to diff viewer when configs are not the same', () => { + const el = shallow(<Diff left={{test: true}} right={{test: false}} />); + expect(el.find('span.removed').length).toBe(1); + expect(el.find('span.added').length).toBe(1); + }); +}); http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/components/__tests__/UpdateDiff-test.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/components/__tests__/UpdateDiff-test.js b/ui/src/main/js/components/__tests__/UpdateDiff-test.js new file mode 100644 index 0000000..c9096c5 --- /dev/null +++ b/ui/src/main/js/components/__tests__/UpdateDiff-test.js @@ -0,0 +1,71 @@ +import React from 'react'; +import { shallow } from 'enzyme'; + +import Diff from '../Diff'; +import UpdateDiff from '../UpdateDiff'; + +import { TaskConfigBuilder } from 'test-utils/TaskBuilders'; +import { + UpdateBuilder, + UpdateDetailsBuilder, + UpdateInstructionsBuilder, + createInstanceTaskGroup +} from 'test-utils/UpdateBuilders'; + +describe('UpdateDiff', () => { + it('Should default to the first group of initialState', () => { + const initialState = [ + createInstanceTaskGroup(TaskConfigBuilder, [0, 0]), + createInstanceTaskGroup(TaskConfigBuilder, [1, 9]), + createInstanceTaskGroup(TaskConfigBuilder, [10, 20]) + ]; + const update = UpdateDetailsBuilder.update( + UpdateBuilder.instructions( + UpdateInstructionsBuilder.initialState(initialState).build()).build()).build(); + const el = shallow(<UpdateDiff update={update} />); + expect(el.contains(<Diff + left={initialState[0].task} + right={update.update.instructions.desiredState.task} />)).toBe(true); + }); + + it('Should not show any dropdown with only one config in initialState', () => { + const initialState = [createInstanceTaskGroup(TaskConfigBuilder, [0, 0])]; + const update = UpdateDetailsBuilder.update( + UpdateBuilder.instructions( + UpdateInstructionsBuilder.initialState(initialState).build()).build()).build(); + const el = shallow(<UpdateDiff update={update} />); + expect(el.find('select').length).toBe(0); + }); + + it('Should show a dropdown when there are more than two configs in initialState', () => { + const initialState = [ + createInstanceTaskGroup(TaskConfigBuilder, [0, 0]), + createInstanceTaskGroup(TaskConfigBuilder, [1, 9]) + ]; + const update = UpdateDetailsBuilder.update( + UpdateBuilder.instructions( + UpdateInstructionsBuilder.initialState(initialState).build()).build()).build(); + const el = shallow(<UpdateDiff update={update} />); + expect(el.find('select').length).toBe(1); + }); + + it('Should update the diff view when you select new groups', () => { + const initialState = [ + createInstanceTaskGroup(TaskConfigBuilder, [0, 0]), + createInstanceTaskGroup(TaskConfigBuilder, [1, 9]), + createInstanceTaskGroup(TaskConfigBuilder, [10, 20]) + ]; + const update = UpdateDetailsBuilder.update( + UpdateBuilder.instructions( + UpdateInstructionsBuilder.initialState(initialState).build()).build()).build(); + const el = shallow(<UpdateDiff update={update} />); + expect(el.contains(<Diff + left={initialState[0].task} + right={update.update.instructions.desiredState.task} />)).toBe(true); + + el.find('select').simulate('change', {target: {value: '2'}}); + expect(el.contains(<Diff + left={initialState[2].task} + right={update.update.instructions.desiredState.task} />)).toBe(true); + }); +}); http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/js/test-utils/UpdateBuilders.js ---------------------------------------------------------------------- diff --git a/ui/src/main/js/test-utils/UpdateBuilders.js b/ui/src/main/js/test-utils/UpdateBuilders.js index 2764f0e..f8ab4d8 100644 --- a/ui/src/main/js/test-utils/UpdateBuilders.js +++ b/ui/src/main/js/test-utils/UpdateBuilders.js @@ -77,6 +77,13 @@ export const UpdateDetailsBuilder = createBuilder({ instanceEvents: [InstanceUpdateEventBuilder.build()] }); +export function createInstanceTaskGroup(taskBuilder, ...instances) { + return { + task: taskBuilder.build(), + instances: instances.map((pair) => { return {first: pair[0], last: pair[1]}; }) + }; +} + export function builderWithStatus(updateStatus) { return UpdateDetailsBuilder.update( UpdateBuilder.summary( http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/sass/app.scss ---------------------------------------------------------------------- diff --git a/ui/src/main/sass/app.scss b/ui/src/main/sass/app.scss index c4b1413..8a10e1c 100644 --- a/ui/src/main/sass/app.scss +++ b/ui/src/main/sass/app.scss @@ -6,6 +6,7 @@ /* Indiviudal Components */ @import 'components/breadcrumb'; +@import 'components/diff'; @import 'components/instance-viz'; @import 'components/navigation'; @import 'components/pagination'; http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/sass/components/_diff.scss ---------------------------------------------------------------------- diff --git a/ui/src/main/sass/components/_diff.scss b/ui/src/main/sass/components/_diff.scss new file mode 100644 index 0000000..f58f7bd --- /dev/null +++ b/ui/src/main/sass/components/_diff.scss @@ -0,0 +1,42 @@ +.task-diff { + font-family: 'Courier', sans-serif; + font-size: 12px; + border: 1px solid $grid_highlight_color; + + .diff-picker { + font-family: $font_stack; + background-color: $grid_color; + border-bottom: 1px solid $grid_highlight_color; + text-transform: uppercase; + padding: 15px 20px 5px 20px; + color: #555; + font-size: 14px; + font-weight: 700; + + div { + font-weight: 700; + margin: 0px 10px; + } + } + + .diff-view { + span { + display: block; + padding: 2px 10px; + width: 100%; + background-color: $content_box_color; + } + + span.same { + background-color: rgba(0, 0, 0, 0.01); + } + + span.removed { + background-color: $colors_error_light; + } + + span.added { + background-color: $colors_success_light; + } + } +} \ No newline at end of file http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/sass/components/_job-page.scss ---------------------------------------------------------------------- diff --git a/ui/src/main/sass/components/_job-page.scss b/ui/src/main/sass/components/_job-page.scss index cd44e36..bafff88 100644 --- a/ui/src/main/sass/components/_job-page.scss +++ b/ui/src/main/sass/components/_job-page.scss @@ -77,49 +77,6 @@ } } - .task-diff { - font-family: 'Courier', sans-serif; - font-size: 12px; - border: 1px solid $grid_highlight_color; - - .diff-picker { - font-family: $font_stack; - background-color: $grid_color; - border-bottom: 1px solid $grid_highlight_color; - text-transform: uppercase; - padding: 15px 20px 5px 20px; - color: #555; - font-size: 14px; - font-weight: 700; - - div { - font-weight: 700; - margin: 0px 10px; - } - } - - .diff-view { - span { - display: block; - padding: 2px 10px; - width: 100%; - background-color: $content_box_color; - } - - span.same { - background-color: rgba(0, 0, 0, 0.01); - } - - span.removed { - background-color: $colors_error_light; - } - - span.added { - background-color: $colors_success_light; - } - } - } - .update-preview { padding: 20px; color: white; http://git-wip-us.apache.org/repos/asf/aurora/blob/0efe4157/ui/src/main/sass/components/_update-page.scss ---------------------------------------------------------------------- diff --git a/ui/src/main/sass/components/_update-page.scss b/ui/src/main/sass/components/_update-page.scss index a0db0b4..c3aefb1 100644 --- a/ui/src/main/sass/components/_update-page.scss +++ b/ui/src/main/sass/components/_update-page.scss @@ -148,4 +148,11 @@ } } } + + .update-diff-picker { + padding: 10px 20px; + background-color: $grid_color; + border-bottom: 1px solid $grid_highlight_color; + font-family: $font_stack; + } } \ No newline at end of file
