Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(756)

Issue 29504594: #2687 - Include mimeo python module (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by f.lopez
Modified:
1 month, 4 weeks ago
Visibility:
Public.

Description

#2687 - Include mimeo python module

Patch Set 1 #

Total comments: 47

Patch Set 2 : For comments 2 to 4 #

Total comments: 14

Patch Set 3 : For comment number 8 #

Total comments: 6

Patch Set 4 : For comment 11 #

Total comments: 22

Patch Set 5 : For comments 14 to 17 #

Total comments: 27

Patch Set 6 : For comments 20 and 21 #

Total comments: 3

Patch Set 7 : For comments 21 and 24 #

Total comments: 2

Patch Set 8 : For comment 26 #

Total comments: 7

Patch Set 9 : For comments 28 to 30 #

Total comments: 28

Patch Set 10 : For comments 33 to 35 #

Total comments: 2

Patch Set 11 : For comment 38 #

Total comments: 8

Patch Set 12 : For comment 41 #

Total comments: 10

Patch Set 13 : For comment 44 #

Total comments: 1

Patch Set 14 : For comments 47 and 48 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+197 lines, -4 lines) Patch
M hiera/roles/web/adblockbrowser.yaml View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +17 lines, -4 lines 0 comments Download
A modules/adblockplus/files/mimeo.py View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +88 lines, -0 lines 0 comments Download
A modules/adblockplus/manifests/web/mimeo.pp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +83 lines, -0 lines 0 comments Download
A modules/adblockplus/templates/mimeo.service.erb View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 51
Fred
https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adblockbrowser.yaml#newcode4 hiera/roles/web/adblockbrowser.yaml:4: aliases: I find it quite inconvenient that you have ...
2 months, 2 weeks ago (2017-08-04 15:20:36 UTC) #1
f.lopez
I'll send another patch later today. https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adblockbrowser.yaml#newcode4 hiera/roles/web/adblockbrowser.yaml:4: aliases: On 2017/08/04 ...
2 months, 2 weeks ago (2017-08-04 16:01:10 UTC) #2
Vasily Kuznetsov
Hi Paco, I looked at the Python part and gave you some general comments (see ...
2 months, 2 weeks ago (2017-08-04 18:01:47 UTC) #3
Vasily Kuznetsov
Hi Paco, I just talked to Matze and he explained to me what this script ...
2 months, 2 weeks ago (2017-08-04 18:24:09 UTC) #4
f.lopez
Thanks for the feedback, I'll send another codereview as soon as possible. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py ...
2 months, 2 weeks ago (2017-08-04 18:58:52 UTC) #5
f.lopez
Incoming codereview https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus/files/mimeo.py#newcode1 modules/adblockplus/files/mimeo.py:1: #!/usr/bin/env python3 On 2017/08/04 18:01:46, Vasily Kuznetsov ...
2 months, 2 weeks ago (2017-08-07 03:10:16 UTC) #6
f.lopez
2 months, 2 weeks ago (2017-08-07 03:11:38 UTC) #7
Vasily Kuznetsov
Hi Paco, Thanks for addressing the comments. There are a few more things that I've ...
2 months, 2 weeks ago (2017-08-07 13:06:41 UTC) #8
f.lopez
Thanks for your comments, I addressed the technical ones, and for the arguments, you are ...
2 months, 2 weeks ago (2017-08-07 18:40:02 UTC) #9
f.lopez
2 months, 2 weeks ago (2017-08-07 18:42:20 UTC) #10
Vasily Kuznetsov
Hey Paco, Almost there. There are a just couple of comments. Also, I haven't run ...
2 months, 2 weeks ago (2017-08-07 20:46:01 UTC) #11
f.lopez
Hey thanks for the quick reply, here it is. :D https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus/files/mimeo.py#newcode18 ...
2 months, 2 weeks ago (2017-08-08 02:48:17 UTC) #12
f.lopez
2 months, 2 weeks ago (2017-08-08 02:48:40 UTC) #13
mathias
https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adblockbrowser.yaml#newcode11 hiera/roles/web/adblockbrowser.yaml:11: proxy_pass http://127.0.0.1:8000; Please explicitly configure the adblockplus::web::mimeo::port above in ...
2 months, 1 week ago (2017-08-08 07:51:46 UTC) #14
Vasily Kuznetsov
https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus/files/mimeo.py#newcode16 modules/adblockplus/files/mimeo.py:16: def get_header_values(self): On 2017/08/08 07:51:46, mathias wrote: > Why ...
2 months, 1 week ago (2017-08-08 11:30:52 UTC) #15
mathias
On 2017/08/08 11:30:52, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus/files/mimeo.py > File modules/adblockplus/files/mimeo.py (right): > > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus/files/mimeo.py#newcode16 ...
2 months, 1 week ago (2017-08-08 11:57:04 UTC) #16
Vasily Kuznetsov
On 2017/08/08 11:57:04, mathias wrote: > On 2017/08/08 11:30:52, Vasily Kuznetsov wrote: > > > ...
2 months, 1 week ago (2017-08-08 16:24:33 UTC) #17
f.lopez
Incoming review https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adblockbrowser.yaml#newcode11 hiera/roles/web/adblockbrowser.yaml:11: proxy_pass http://127.0.0.1:8000; On 2017/08/08 07:51:45, mathias wrote: ...
2 months, 1 week ago (2017-08-09 19:50:49 UTC) #18
f.lopez
2 months, 1 week ago (2017-08-09 19:51:13 UTC) #19
mathias
We're getting there ;) https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus/files/mimeo.py#newcode14 modules/adblockplus/files/mimeo.py:14: LOCK = threading.Lock() Not sure ...
2 months, 1 week ago (2017-08-09 21:09:08 UTC) #20
Vasily Kuznetsov
Hi guys, I'm gonna add my 2 Rappen to the Python part of the review. ...
2 months, 1 week ago (2017-08-10 15:05:58 UTC) #21
f.lopez
incoming review https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus/files/mimeo.py#newcode14 modules/adblockplus/files/mimeo.py:14: LOCK = threading.Lock() On 2017/08/09 21:09:07, mathias ...
2 months, 1 week ago (2017-08-10 19:06:35 UTC) #22
f.lopez
2 months, 1 week ago (2017-08-10 19:06:48 UTC) #23
Vasily Kuznetsov
Hi Paco! I have some more ideas :) Here we go: https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): ...
2 months, 1 week ago (2017-08-11 17:31:48 UTC) #24
f.lopez
2 months, 1 week ago (2017-08-13 16:13:07 UTC) #25
Vasily Kuznetsov
Hey Paco, Thanks for looking into the comments. Here's some more :) Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus/files/mimeo.py ...
2 months, 1 week ago (2017-08-14 10:59:56 UTC) #26
f.lopez
2 months, 1 week ago (2017-08-14 19:01:41 UTC) #27
Vasily Kuznetsov
Hi Paco, It seems like the exception handling/logging is still not quite right (see below) ...
2 months ago (2017-08-15 18:56:26 UTC) #28
f.lopez
I find counterintuitive to do it separately, I can be wrong but what do you ...
2 months ago (2017-08-16 00:54:32 UTC) #29
Vasily Kuznetsov
https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus/files/mimeo.py#newcode42 modules/adblockplus/files/mimeo.py:42: self.send_simple_response(500) On 2017/08/16 00:54:32, f.lopez wrote: > On 2017/08/15 ...
2 months ago (2017-08-16 11:32:27 UTC) #30
f.lopez
2 months ago (2017-08-16 16:14:15 UTC) #31
Vasily Kuznetsov
Hi Paco, Assuming you've tested this code, LGTM. Cheers, Vasily
2 months ago (2017-08-17 11:08:37 UTC) #32
mathias
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py#newcode24 modules/adblockplus/files/mimeo.py:24: values[name[1:]] = self.headers.get(new_var, '-') Does this understand translation between ...
2 months ago (2017-08-18 08:47:47 UTC) #33
Vasily Kuznetsov
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py#newcode79 modules/adblockplus/files/mimeo.py:79: fh = sys.stdout On 2017/08/18 08:47:43, mathias wrote: > ...
2 months ago (2017-08-18 12:49:58 UTC) #34
mathias
On 2017/08/18 12:49:58, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py > File modules/adblockplus/files/mimeo.py (right): > > https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py#newcode79 ...
2 months ago (2017-08-18 14:26:58 UTC) #35
f.lopez
I'm sending a new review and wait for your comments on how we should handle ...
2 months ago (2017-08-19 00:51:03 UTC) #36
f.lopez
2 months ago (2017-08-19 00:53:46 UTC) #37
mathias
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus/files/mimeo.py#newcode24 modules/adblockplus/files/mimeo.py:24: values[name[1:]] = self.headers.get(new_var, '-') On 2017/08/19 00:51:00, f.lopez wrote: ...
2 months ago (2017-08-19 15:06:09 UTC) #38
f.lopez
Ok I get it now, so when you suggested the backticks (not backquotes, my bad) ...
2 months ago (2017-08-20 23:49:56 UTC) #39
f.lopez
2 months ago (2017-08-20 23:51:41 UTC) #40
mathias
Getting closer.. :) https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus/files/mimeo.py#newcode87 modules/adblockplus/files/mimeo.py:87: except: Shouldn't this be `finally`? https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus/manifests/web/mimeo.pp ...
2 months ago (2017-08-21 07:37:44 UTC) #41
f.lopez
https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus/files/mimeo.py File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus/files/mimeo.py#newcode87 modules/adblockplus/files/mimeo.py:87: except: On 2017/08/21 07:37:42, mathias wrote: > Shouldn't this ...
2 months ago (2017-08-21 19:54:13 UTC) #42
f.lopez
2 months ago (2017-08-21 19:54:38 UTC) #43
mathias
Almost! https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adblockbrowser.yaml#newcode20 hiera/roles/web/adblockbrowser.yaml:20: mimeo_data: Sorry, but since it's hard-coded in the ...
2 months ago (2017-08-21 20:07:40 UTC) #44
f.lopez
Incoming review https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adblockbrowser.yaml File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adblockbrowser.yaml#newcode20 hiera/roles/web/adblockbrowser.yaml:20: mimeo_data: On 2017/08/21 20:07:38, mathias wrote: > ...
1 month, 4 weeks ago (2017-08-22 16:52:44 UTC) #45
f.lopez
1 month, 4 weeks ago (2017-08-22 16:52:59 UTC) #46
mathias
https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus/manifests/web/mimeo.pp File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus/manifests/web/mimeo.pp#newcode74 modules/adblockplus/manifests/web/mimeo.pp:74: File['/var/adblockplus/mimeo'], The directory is an implicit dependency (via mimeo.py), ...
1 month, 4 weeks ago (2017-08-22 17:30:58 UTC) #47
mathias
On 2017/08/22 17:30:58, mathias wrote: > https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus/manifests/web/mimeo.pp > File modules/adblockplus/manifests/web/mimeo.pp (right): > > https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus/manifests/web/mimeo.pp#newcode74 > ...
1 month, 4 weeks ago (2017-08-22 17:32:16 UTC) #48
f.lopez
1 month, 4 weeks ago (2017-08-22 20:33:28 UTC) #49
mathias
LGTM.
1 month, 4 weeks ago (2017-08-22 21:22:52 UTC) #50
Vasily Kuznetsov
1 month, 4 weeks ago (2017-08-23 09:22:00 UTC) #51
Also LGTM for 'mimeo.py' in patch set 14.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5