You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by David McLaughlin <da...@dmclaughlin.com> on 2017/10/17 20:11:19 UTC
Review Request 63083: Add diff viewer to Update Page
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/
-----------------------------------------------------------
Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
Repository: aurora
Description
-------
Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
Diffs
-----
ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
ui/src/main/js/components/Diff.js PRE-CREATION
ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
ui/src/main/js/components/UpdateDiff.js PRE-CREATION
ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
ui/src/main/sass/components/_diff.scss PRE-CREATION
ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
Diff: https://reviews.apache.org/r/63083/diff/1/
Testing
-------
./gradlew ui:lint
./gradlew ui:test
See screenshots.
File Attachments
----------------
Selecting from multiple initial configs
https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
Showing diff by default
https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
Thanks,
David McLaughlin
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188380
-----------------------------------------------------------
Ship it!
Master (94999eb) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Oct. 17, 2017, 8:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 8:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/
-----------------------------------------------------------
(Updated Oct. 17, 2017, 9:57 p.m.)
Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
Changes
-------
Rebase before push.
Repository: aurora
Description
-------
Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
Diffs (updated)
-----
ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
ui/src/main/js/components/Diff.js PRE-CREATION
ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
ui/src/main/js/components/UpdateDiff.js PRE-CREATION
ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
ui/src/main/sass/app.scss c4b14135b4721327436db14159387ea0ea335a41
ui/src/main/sass/components/_diff.scss PRE-CREATION
ui/src/main/sass/components/_job-page.scss cd44e36df37aecf650f7df4c9f2d27ce9121b9ce
ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
Diff: https://reviews.apache.org/r/63083/diff/2/
Changes: https://reviews.apache.org/r/63083/diff/1-2/
Testing
-------
./gradlew ui:lint
./gradlew ui:test
See screenshots.
File Attachments
----------------
Selecting from multiple initial configs
https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
Showing diff by default
https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
Thanks,
David McLaughlin
Re: Review Request 63083: Add diff viewer to Update Page
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Oct. 17, 2017, 9:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
>
> David McLaughlin wrote:
> Currently that would have to assume the executor config is JSON.
>
> David McLaughlin wrote:
> But my long term plan is to look for "AuroraExecutor" in the config and do nice things for Thermos (and allow other people to drop in their own parsers for custom executors).
>
> Stephan Erb wrote:
> That is why I'd propose to do it here: https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/config/thrift.py#L328
>
> At that point in time we are sure this is a Thermos config in json. As it is just a blob for Aurora, formatting it should not really matter.
>
> Stephan Erb wrote:
> I agree thought hat a specific `"AuroraExecutor"` mode would be the best thing. My proposal is more like a very easy workaround.
Even if it's pretty printed, the issue is that because it's a string the entire string is considered a diff and you will just see two huge red/green blobs in the output. What we want is to deserialize the JSON and only see the parts which have actually change. All we need is the confidence to do JSON.parse on the executor data.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 8:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 8:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Stephan Erb <se...@apache.org>.
> On Oct. 17, 2017, 11:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
>
> David McLaughlin wrote:
> Currently that would have to assume the executor config is JSON.
>
> David McLaughlin wrote:
> But my long term plan is to look for "AuroraExecutor" in the config and do nice things for Thermos (and allow other people to drop in their own parsers for custom executors).
>
> Stephan Erb wrote:
> That is why I'd propose to do it here: https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/config/thrift.py#L328
>
> At that point in time we are sure this is a Thermos config in json. As it is just a blob for Aurora, formatting it should not really matter.
>
> Stephan Erb wrote:
> I agree thought hat a specific `"AuroraExecutor"` mode would be the best thing. My proposal is more like a very easy workaround.
>
> David McLaughlin wrote:
> Even if it's pretty printed, the issue is that because it's a string the entire string is considered a diff and you will just see two huge red/green blobs in the output. What we want is to deserialize the JSON and only see the parts which have actually change. All we need is the confidence to do JSON.parse on the executor data.
Ah alright. Thanks for clarifying.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 11:57 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 11:57 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss c4b14135b4721327436db14159387ea0ea335a41
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss cd44e36df37aecf650f7df4c9f2d27ce9121b9ce
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/2/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Stephan Erb <se...@apache.org>.
> On Oct. 17, 2017, 11:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
>
> David McLaughlin wrote:
> Currently that would have to assume the executor config is JSON.
>
> David McLaughlin wrote:
> But my long term plan is to look for "AuroraExecutor" in the config and do nice things for Thermos (and allow other people to drop in their own parsers for custom executors).
That is why I'd propose to do it here: https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/config/thrift.py#L328
At that point in time we are sure this is a Thermos config in json. As it is just a blob for Aurora, formatting it should not really matter.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 10:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 10:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Oct. 17, 2017, 9:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
Currently that would have to assume the executor config is JSON.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 8:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 8:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Stephan Erb <se...@apache.org>.
> On Oct. 17, 2017, 11:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
>
> David McLaughlin wrote:
> Currently that would have to assume the executor config is JSON.
>
> David McLaughlin wrote:
> But my long term plan is to look for "AuroraExecutor" in the config and do nice things for Thermos (and allow other people to drop in their own parsers for custom executors).
>
> Stephan Erb wrote:
> That is why I'd propose to do it here: https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/config/thrift.py#L328
>
> At that point in time we are sure this is a Thermos config in json. As it is just a blob for Aurora, formatting it should not really matter.
I agree thought hat a specific `"AuroraExecutor"` mode would be the best thing. My proposal is more like a very easy workaround.
- Stephan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 10:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 10:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Oct. 17, 2017, 9:03 p.m., Stephan Erb wrote:
> > Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
>
> David McLaughlin wrote:
> Currently that would have to assume the executor config is JSON.
But my long term plan is to look for "AuroraExecutor" in the config and do nice things for Thermos (and allow other people to drop in their own parsers for custom executors).
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
On Oct. 17, 2017, 8:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 8:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Stephan Erb <se...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188390
-----------------------------------------------------------
Should we consider pretty printing the executor configuration (when assembling the thrift struct in the client)? Hopefully this would yield diffs that are a bit easier to read.
- Stephan Erb
On Oct. 17, 2017, 10:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 10:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>
Re: Review Request 63083: Add diff viewer to Update Page
Posted by Santhosh Kumar Shanmugham <sa...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/63083/#review188387
-----------------------------------------------------------
Ship it!
Ship It!
- Santhosh Kumar Shanmugham
On Oct. 17, 2017, 1:11 p.m., David McLaughlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63083/
> -----------------------------------------------------------
>
> (Updated Oct. 17, 2017, 1:11 p.m.)
>
>
> Review request for Aurora, Kai Huang, Reza Motamedi, and Santhosh Kumar Shanmugham.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Add diff viewer from Job Page to Update Page. Key difference on update page is that the "after" diff is fixed - it's always the desiredState. But I've extracted the Diff rendering into a shared component between ConfigDiff and UpdateDiff, as well as the CSS.
>
>
> Diffs
> -----
>
> ui/src/main/js/components/ConfigDiff.js 9627751293ff01c9a3b728e2c352894f1a1e22f3
> ui/src/main/js/components/Diff.js PRE-CREATION
> ui/src/main/js/components/InstanceViz.js 99efec4c15fcc677820b3145f506997ef8a37207
> ui/src/main/js/components/UpdateConfig.js fac3c8856f3fdcb918fe82ac7d61cf0e53b23756
> ui/src/main/js/components/UpdateDiff.js PRE-CREATION
> ui/src/main/js/components/__tests__/ConfigDiff-test.js 3eaa78a9660e1e4bac284d545f5fae05c24e93f7
> ui/src/main/js/components/__tests__/Diff-test.js PRE-CREATION
> ui/src/main/js/components/__tests__/UpdateDiff-test.js PRE-CREATION
> ui/src/main/js/test-utils/UpdateBuilders.js 2764f0e1d6446e5b3aa99d1dc1a952a20efa4798
> ui/src/main/sass/app.scss 3a799b658ad6c94144cda7beec38c4b72ec393d8
> ui/src/main/sass/components/_diff.scss PRE-CREATION
> ui/src/main/sass/components/_job-page.scss 8523fcf4c917b46c9d514325f073bd5788798156
> ui/src/main/sass/components/_update-page.scss a0db0b43b61e8ef10c29195945e7543a982aeab1
>
>
> Diff: https://reviews.apache.org/r/63083/diff/1/
>
>
> Testing
> -------
>
> ./gradlew ui:lint
> ./gradlew ui:test
>
> See screenshots.
>
>
> File Attachments
> ----------------
>
> Selecting from multiple initial configs
> https://reviews.apache.org/media/uploaded/files/2017/10/17/df437cfa-a97c-4aa1-9a95-2e4291020b9b__Screen_Shot_2017-10-17_at_11.22.18_AM.png
> Showing diff by default
> https://reviews.apache.org/media/uploaded/files/2017/10/17/740cc0a0-9ac7-413a-8a33-fe452f998400__Screen_Shot_2017-10-17_at_1.09.32_PM.png
>
>
> Thanks,
>
> David McLaughlin
>
>