Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29762573: Issue 6602 - Introduce watchextensions

Created:
April 26, 2018, 11:03 a.m. by tlucas
Modified:
May 18, 2018, 8:19 a.m.
CC:
kzar
Visibility:
Public.

Description

Issue 6602 - Introduce watchextensions Note: this will be pushed into a new repository

Patch Set 1 #

Patch Set 2 : Actual Implementation for 6602 #

Patch Set 3 : #

Total comments: 19

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+502 lines, -0 lines) Patch
A .gitignore View 1 1 chunk +5 lines, -0 lines 0 comments Download
A README.md View 1 1 chunk +65 lines, -0 lines 0 comments Download
A tests/data/omaha.json View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
A tests/test_ExtWatch.py View 1 2 3 1 chunk +129 lines, -0 lines 0 comments Download
A tox.ini View 1 1 chunk +33 lines, -0 lines 0 comments Download
A watchextensions.py View 1 2 3 1 chunk +260 lines, -0 lines 0 comments Download
A watchextensions.ini.example View 1 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 8
tlucas
Neither the code, nor the list of reviewers / ccs is final. Yet todo: * ...
April 26, 2018, 11:07 a.m. (2018-04-26 11:07:05 UTC) #1
tlucas
On 2018/04/26 11:07:05, tlucas wrote: > Neither the code, nor the list of reviewers / ...
April 26, 2018, 1:09 p.m. (2018-04-26 13:09:55 UTC) #2
tlucas
Hi guys. This is my attempt to implement #6602, any comments would be welcome. I ...
May 7, 2018, 3:03 p.m. (2018-05-07 15:03:34 UTC) #3
tlucas
Patch Set 3 * Don't log sensitive data (i.e. auth tokens in repository-url)
May 8, 2018, 2:23 p.m. (2018-05-08 14:23:35 UTC) #4
Vasily Kuznetsov
Hi Tristan, Pretty cool, I like the script. A few questions/points: - I'm not sure ...
May 15, 2018, 4:49 p.m. (2018-05-15 16:49:32 UTC) #5
tlucas
Patch Set 4 * Addressing Vasily's comments Hey, thanks for your comments! On 2018/05/15 16:49:32, ...
May 16, 2018, 9:52 a.m. (2018-05-16 09:52:38 UTC) #6
Vasily Kuznetsov
Hi Tristan, Thanks for addressing my comments -- looks good. I think we should still ...
May 16, 2018, 5:54 p.m. (2018-05-16 17:54:55 UTC) #7
tlucas
May 18, 2018, 8:19 a.m. (2018-05-18 08:19:41 UTC) #8
On 2018/05/16 17:54:55, Vasily Kuznetsov wrote:
> Hi Tristan,
> 
> Thanks for addressing my comments -- looks good.
> 
> I think we should still think if we really want to support Python 2 with this
> code, and if we decide that we don't, some things could be modernized and
> simplified. I don't really see much going for Python 2 support, but I
definitely
> don't mind if there are good reasons to do so. Sebastian, Dave: what do you
> think?
> 
> Cheers,
> Vasily
> 
> P.S. You can go to https://pythonclock.org/ and meditate on it a bit to start
> agreeing more with me about Python 2 ;)
> 
> https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py
> File watchextensions.py (right):
> 
>
https://codereview.adblockplus.org/29762573/diff/29774558/watchextensions.py#...
> watchextensions.py:43: class SensitiveFilter(logging.Filter):
> On 2018/05/16 09:52:38, tlucas wrote:
> > On 2018/05/15 16:49:32, Vasily Kuznetsov wrote:
> > > If I understand it correctly, the purpose of this class is to prevent
> > > security-sensitive data from the repository string in the the config from
> > > getting into the logs. First of all, it takes a bit of time to figure this
> out
> > > so perhaps it would be good to document the class. Second: could we
perhaps
> do
> > > it with less code somehow. What if we just don't include the full
repository
> > > string into error messages, etc. Could this be simpler?
> > 
> > You are right about the purpose - I added a documentation for this class.
> > 
> > However, the amount of code is - from what i see - as minimal as it can be
> (FWIW
> > i could get rid of self.mask() and put the logic in self.filter(), but
that's
> > about it i guess - if we don't want to lose ALL information, that is.)
> 
> I was thinking about avoiding this filter class altogether and making sure
that
> nothing sensitive ends up in the log by just not writing it into the log in
the
> first place. I'm not sure if this would be easier, but I trust that you can
> figure it out :)
> 
> In any case, as it is now, the code is easy enough to follow so it's not a big
> deal.

I guess the only other entry point would be "subprocess" not printing it's
command the stderr on failure. And TBH i once tried to filter stuff there, but
IMHO it was not feasible if we still want to get some sort of idea what went
wrong - so if nobody minds, i'll stick to the current implementation :)

Let's see where this go about Python 2 / 3 though.

Powered by Google App Engine
This is Rietveld