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 Robinson <dr...@twitter.com> on 2014/07/08 03:08:41 UTC
Review Request 23329: remove embedded jquery
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Brian Wickman.
Bugs: AURORA-578
https://issues.apache.org/jira/browse/AURORA-578
Repository: aurora
Description
-------
remove embedded jquery
Diffs
-----
src/main/python/apache/thermos/observer/http/assets/jquery.js 3774ff986139c8a7534e14bc8987fe80418dcc1b
src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
Diff: https://reviews.apache.org/r/23329/diff/
Testing
-------
Thanks,
David Robinson
Re: Review Request 23329: remove embedded jquery
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review58218
-----------------------------------------------------------
This patch does not apply cleanly on master (53f4e73), do you need to rebase?
- Aurora ReviewBot
On July 9, 2014, 9:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 9:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by David Robinson <dr...@twitter.com>.
> On July 30, 2014, 4:25 a.m., David McLaughlin wrote:
> > Not sure I understand the motivation here. It's generally best to host your own static assets.
Users typically debug their Aurora jobs by looking at individual instances, which usually involves accessing the web interfaces (provided by the Thermos Observer) of several (or numerous) slaves. When users access the web interface of different slaves their browser will download jquery.js from each slave visited, unless it already has it cached (they've previously visited the same slave). The browser's cache hit-rate for jquery.js is practically zero because each slave hosts to its own copy. If we instead point to a version stored on a CDN then it's likely that the browser will already have jquery in its cache, therefore the request can be served immediately, which will make the UI slightly faster and save a small amount of bandwidth.
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review49084
-----------------------------------------------------------
On July 9, 2014, 9:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 9:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review49084
-----------------------------------------------------------
Not sure I understand the motivation here. It's generally best to host your own static assets.
- David McLaughlin
On July 9, 2014, 9:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 9:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by Jake Farrell <jf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review49153
-----------------------------------------------------------
./src/main/python/apache/thermos/observer/http/assets/jquery* should be removed in favor of using 3rdparty bower components (these are the last deps in the src tree that are not in 3rdparty currently)
- Jake Farrell
On July 9, 2014, 9:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 9:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review49045
-----------------------------------------------------------
Reviewers - ping?
- Bill Farner
On July 9, 2014, 9:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 9:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review54897
-----------------------------------------------------------
Can this be discarded?
- Kevin Sweeney
On July 9, 2014, 2:50 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 9, 2014, 2:50 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by David Robinson <dr...@twitter.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/
-----------------------------------------------------------
(Updated July 9, 2014, 9:50 p.m.)
Review request for Aurora, David McLaughlin and Brian Wickman.
Changes
-------
Fall back to local jquery.
Bugs: AURORA-578
https://issues.apache.org/jira/browse/AURORA-578
Repository: aurora
Description
-------
remove embedded jquery
Diffs (updated)
-----
src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
Diff: https://reviews.apache.org/r/23329/diff/
Testing
-------
Thanks,
David Robinson
Re: Review Request 23329: remove embedded jquery
Posted by David Robinson <dr...@twitter.com>.
> On July 8, 2014, 1:18 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl, line 62
> > <https://reviews.apache.org/r/23329/diff/1/?file=625207#file625207line62>
> >
> > -1, this is pretty much unacceptable from a security standpoint - data visible to the observer origin includes sensitive application logs
> >
> > -1 from a reliability standpoint as well - the observer is used to debug low-level infrastructure and a dependency on an external CDN doesn't work for that.
Thermos serves its static content (including the bundled jquery) and logs over http, in clear text... that's a bigger security risk, no?
I understand why you wouldn't trust any old CDN, but why can't we trust Google's?
We could instead use the local copy as a fallback:
<script src="//ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js"></script>
<script>window.jQuery || document.write('<script src="/assets/jquery.js">\x3C/script>')</script>
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review47428
-----------------------------------------------------------
On July 8, 2014, 1:08 a.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 8, 2014, 1:08 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/assets/jquery.js 3774ff986139c8a7534e14bc8987fe80418dcc1b
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>
Re: Review Request 23329: remove embedded jquery
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23329/#review47428
-----------------------------------------------------------
src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl
<https://reviews.apache.org/r/23329/#comment83209>
-1, this is pretty much unacceptable from a security standpoint - data visible to the observer origin includes sensitive application logs
-1 from a reliability standpoint as well - the observer is used to debug low-level infrastructure and a dependency on an external CDN doesn't work for that.
- Kevin Sweeney
On July 7, 2014, 6:08 p.m., David Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23329/
> -----------------------------------------------------------
>
> (Updated July 7, 2014, 6:08 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Brian Wickman.
>
>
> Bugs: AURORA-578
> https://issues.apache.org/jira/browse/AURORA-578
>
>
> Repository: aurora
>
>
> Description
> -------
>
> remove embedded jquery
>
>
> Diffs
> -----
>
> src/main/python/apache/thermos/observer/http/assets/jquery.js 3774ff986139c8a7534e14bc8987fe80418dcc1b
> src/main/python/apache/thermos/observer/http/templates/filebrowse.tpl 511d7c06206ae5fd8a4206683f09348e1276b8c4
> src/main/python/apache/thermos/observer/http/templates/index.tpl 3ccb6e841c932cb8bcb43b765e0b5aa8bc567f88
> src/main/python/apache/thermos/observer/http/templates/logbrowse.tpl b182a4b331fbe8b9dd437194d195d220184a2f7c
>
> Diff: https://reviews.apache.org/r/23329/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> David Robinson
>
>