aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephan Erb <s...@apache.org>
Subject Re: Review Request 63083: Add diff viewer to Update Page
Date Tue, 17 Oct 2017 22:03:22 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message