Issue 29504594: #2687 - Include mimeo python module (Closed)
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 #
MessagesTotal messages: 51
https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:4: aliases: I find it quite inconvenient that you have to specify the strings in this ugly systemd-escaped format here. Can't you handle the systemd-escaping internally somehow? https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:11: } Why do you hardcode the port here if you provide a parameter for it in the mimeo class? https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:37: $log_file = '/var/log/mimeo.py', Isn't .py a confusing line ending for a log file? I suggest naming the default log file /var/log/mimeo.log
I'll send another patch later today. https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:4: aliases: On 2017/08/04 15:20:36, Fred wrote: > I find it quite inconvenient that you have to specify the strings in this ugly > systemd-escaped format here. > Can't you handle the systemd-escaping internally somehow? I can but this means an extra step with an `exec` resource, I'll try it out and see what comes from it https://codereview.adblockplus.org/29504594/diff/29504595/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:11: } On 2017/08/04 15:20:36, Fred wrote: > Why do you hardcode the port here if you provide a parameter for it in the mimeo > class? These are two different configurations in different modules, here one should specify custom nginx configuration outside of the scope of the normal modules, there is a lot of extra complexity in order to add the port to this configuration template which I think it's not worth the effort. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:37: $log_file = '/var/log/mimeo.py', On 2017/08/04 15:20:36, Fred wrote: > Isn't .py a confusing line ending for a log file? > I suggest naming the default log file /var/log/mimeo.log My bad, I actually meant `.log` instead of `.py`
Hi Paco, I looked at the Python part and gave you some general comments (see below). It seems that the ticket, whose number is given in the review title (http://hub.eyeo.com/issues/622, or is it https://issues.adblockplus.org/ticket/622 ?) is about something else, so I don't know what exactly you're trying to do with this code. When I understand that better, I will probably have more specific feedback! Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:1: #!/usr/bin/env python3 Yay! Python 3 FTW! https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:9: punctuation = """!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~""" These things look like constants, they should be named in UPPER_CASE. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: class Handler(BaseHTTPRequestHandler): PEP8 recommends to surround top level classes and functions with 2 blank lines [1]. Also, you can use flake8 utility[2] (probably there's a plugin for Emacs as well) to spot such issues for you. We also have a plugin[3] that spots deviations from eyeo Python coding styleguide[4]. [1]: https://www.python.org/dev/peps/pep-0008/#blank-lines [2]: http://flake8.pycqa.org/ [3]: https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo [4]: https://adblockplus.org/coding-style#python https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:13: def set_values(self, values, log_format): So here you take a dict of variables as a parameter, then modify it (by adding the variables that are used in `log_format`) and finally return the result. It might seem like you don't change the dict that was passed, since the function is returning a dict, so I would say that this kind of API is rather confusing and error-prone. A more clear option would be to not return anything, making it clear that the original dict will be modified. However, I think there's an even cleaner solution: - Don't pass values to `set_values`, but make a new dict in it. - Fill this new dict with whatever you extract from the headers based on the `log_format`. - Return the new dict and then outside do a `values.update(self.set_values(log_format))`. What do you think? If you follow my suggestion, it might also make sense to rename the function to `get_header_values` or something like that. You also only call it once with `self.log_format` as a parameter. Maybe you don't need this parameter, since you already have `self` inside anyway? https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: name = var.rstrip(punctuation).lstrip(punctuation) This seems to be equivalent to `var.strip(punctuation)`, right? https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:17: new_var = name[len(proxy_token):].title() You seem to be assuming that `proxy_token` is at the beginning of `name`, but what if it's not? For example if `log_format` is '{foo_http_bar}'? Perhaps it would be safer to do something like this: for var in log_format.split(): name = var.strip(...) if name.startswith(proxy_token): # here we're sure that name starts with 'http_'. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:21: def log_mensaje(self, form, args): I'd suggest to name this in English. Not everyone speaks Spanish ;) https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:40: self.send_header("Content-Type",values["Content-Type"]) Our style guide recommends using single quotes in single-line strings that don't contain a single quote inside. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:46: def run(server_class=HTTPServer, handler_class=Handler, port=8000, log_format=''): Do you really need this function? There's a lot of parameter passing, defaults and stuff, but unless somebody is going to import this module and call `run()`, it doesn't seem to be worth it. Maybe just inline its content where it's called below? https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:53: parser.add_argument('port', action='store', Why is "port" a positional argument while all others are keyword arguments? This seems a bit arbitrary. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:57: parser.add_argument('--response', action='store', This is an optional parameter with no default. If it's not supplied, wouldn't `args.response` be `None`? https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:62: help='Specify the format of the log ouput') I don't think you need the word "Specify" here. The `help` argument is a description of the argument, so I don't think it should be an imperative sentence. For the other arguments you are doing it right IMO. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:68: logging.basicConfig(filename=args.log_file, level=logging.INFO) What do you think about moving this line 2 lines down, so that it's just before `run(...)`? It would be nicer if all the stuff related to `handler_class` was together.
Hi Paco, I just talked to Matze and he explained to me what this script is doing and how it's connected to the ticket. It makes more sense now (although it would be better if it was obvious from the ticket, why this is needed). We had a short chat about the best way to organize the arguments and concluded that it makes sense for the log file (with default behavior to output everythong to stdout) to be the only positional argument and for everything else to be keyword arguments. Cheers, Vasily
Thanks for the feedback, I'll send another codereview as soon as possible. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:9: punctuation = """!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~""" On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > These things look like constants, they should be named in UPPER_CASE. Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: class Handler(BaseHTTPRequestHandler): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > PEP8 recommends to surround top level classes and functions with 2 blank lines > [1]. > > Also, you can use flake8 utility[2] (probably there's a plugin for Emacs as > well) to spot such issues for you. We also have a plugin[3] that spots > deviations from eyeo Python coding styleguide[4]. > > [1]: https://www.python.org/dev/peps/pep-0008/#blank-lines > [2]: http://flake8.pycqa.org/ > [3]: https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo > [4]: https://adblockplus.org/coding-style#python Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:13: def set_values(self, values, log_format): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > So here you take a dict of variables as a parameter, then modify it (by adding > the variables that are used in `log_format`) and finally return the result. It > might seem like you don't change the dict that was passed, since the function is > returning a dict, so I would say that this kind of API is rather confusing and > error-prone. A more clear option would be to not return anything, making it > clear that the original dict will be modified. > > However, I think there's an even cleaner solution: > - Don't pass values to `set_values`, but make a new dict in it. > - Fill this new dict with whatever you extract from the headers based on the > `log_format`. > - Return the new dict and then outside do a > `values.update(self.set_values(log_format))`. > What do you think? > > If you follow my suggestion, it might also make sense to rename the function to > `get_header_values` or something like that. > > You also only call it once with `self.log_format` as a parameter. Maybe you > don't need this parameter, since you already have `self` inside anyway? Ok this makes way more sense, thanks https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: name = var.rstrip(punctuation).lstrip(punctuation) On 2017/08/04 18:01:45, Vasily Kuznetsov wrote: > This seems to be equivalent to `var.strip(punctuation)`, right? Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:17: new_var = name[len(proxy_token):].title() On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > You seem to be assuming that `proxy_token` is at the beginning of `name`, but > what if it's not? For example if `log_format` is '{foo_http_bar}'? Perhaps it > would be safer to do something like this: > > for var in log_format.split(): > name = var.strip(...) > if name.startswith(proxy_token): > # here we're sure that name starts with 'http_'. you are right. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:21: def log_mensaje(self, form, args): On 2017/08/04 18:01:47, Vasily Kuznetsov wrote: > I'd suggest to name this in English. Not everyone speaks Spanish ;) Ok I did change it but apparently I forgot to push it to the code review, thanks for pointing it out. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:40: self.send_header("Content-Type",values["Content-Type"]) On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Our style guide recommends using single quotes in single-line strings that don't > contain a single quote inside. Gonna use what you suggested before for the code standards. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:46: def run(server_class=HTTPServer, handler_class=Handler, port=8000, log_format=''): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Do you really need this function? There's a lot of parameter passing, defaults > and stuff, but unless somebody is going to import this module and call `run()`, > it doesn't seem to be worth it. Maybe just inline its content where it's called > below? Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:53: parser.add_argument('port', action='store', On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Why is "port" a positional argument while all others are keyword arguments? This > seems a bit arbitrary. Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:57: parser.add_argument('--response', action='store', On 2017/08/04 18:01:45, Vasily Kuznetsov wrote: > This is an optional parameter with no default. If it's not supplied, wouldn't > `args.response` be `None`? Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:62: help='Specify the format of the log ouput') On 2017/08/04 18:01:47, Vasily Kuznetsov wrote: > I don't think you need the word "Specify" here. The `help` argument is a > description of the argument, so I don't think it should be an imperative > sentence. For the other arguments you are doing it right IMO. Acknowledged. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:68: logging.basicConfig(filename=args.log_file, level=logging.INFO) On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > What do you think about moving this line 2 lines down, so that it's just before > `run(...)`? It would be nicer if all the stuff related to `handler_class` was > together. I'm not using the function anymore but I'll still move this line.
Incoming codereview https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:1: #!/usr/bin/env python3 On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Yay! Python 3 FTW! Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:9: punctuation = """!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~""" On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > These things look like constants, they should be named in UPPER_CASE. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:9: punctuation = """!"#$%&'()*+,-./:;<=>?@[\]^_`{|}~""" On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > These things look like constants, they should be named in UPPER_CASE. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: class Handler(BaseHTTPRequestHandler): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > PEP8 recommends to surround top level classes and functions with 2 blank lines > [1]. > > Also, you can use flake8 utility[2] (probably there's a plugin for Emacs as > well) to spot such issues for you. We also have a plugin[3] that spots > deviations from eyeo Python coding styleguide[4]. > > [1]: https://www.python.org/dev/peps/pep-0008/#blank-lines > [2]: http://flake8.pycqa.org/ > [3]: https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo > [4]: https://adblockplus.org/coding-style#python Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: class Handler(BaseHTTPRequestHandler): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > PEP8 recommends to surround top level classes and functions with 2 blank lines > [1]. > > Also, you can use flake8 utility[2] (probably there's a plugin for Emacs as > well) to spot such issues for you. We also have a plugin[3] that spots > deviations from eyeo Python coding styleguide[4]. > > [1]: https://www.python.org/dev/peps/pep-0008/#blank-lines > [2]: http://flake8.pycqa.org/ > [3]: https://hg.adblockplus.org/codingtools/file/tip/flake8-eyeo > [4]: https://adblockplus.org/coding-style#python Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:13: def set_values(self, values, log_format): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > So here you take a dict of variables as a parameter, then modify it (by adding > the variables that are used in `log_format`) and finally return the result. It > might seem like you don't change the dict that was passed, since the function is > returning a dict, so I would say that this kind of API is rather confusing and > error-prone. A more clear option would be to not return anything, making it > clear that the original dict will be modified. > > However, I think there's an even cleaner solution: > - Don't pass values to `set_values`, but make a new dict in it. > - Fill this new dict with whatever you extract from the headers based on the > `log_format`. > - Return the new dict and then outside do a > `values.update(self.set_values(log_format))`. > What do you think? > > If you follow my suggestion, it might also make sense to rename the function to > `get_header_values` or something like that. > > You also only call it once with `self.log_format` as a parameter. Maybe you > don't need this parameter, since you already have `self` inside anyway? Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: name = var.rstrip(punctuation).lstrip(punctuation) On 2017/08/04 18:01:45, Vasily Kuznetsov wrote: > This seems to be equivalent to `var.strip(punctuation)`, right? Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: name = var.rstrip(punctuation).lstrip(punctuation) On 2017/08/04 18:01:45, Vasily Kuznetsov wrote: > This seems to be equivalent to `var.strip(punctuation)`, right? Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:17: new_var = name[len(proxy_token):].title() On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > You seem to be assuming that `proxy_token` is at the beginning of `name`, but > what if it's not? For example if `log_format` is '{foo_http_bar}'? Perhaps it > would be safer to do something like this: > > for var in log_format.split(): > name = var.strip(...) > if name.startswith(proxy_token): > # here we're sure that name starts with 'http_'. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:21: def log_mensaje(self, form, args): On 2017/08/04 18:01:47, Vasily Kuznetsov wrote: > I'd suggest to name this in English. Not everyone speaks Spanish ;) Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:40: self.send_header("Content-Type",values["Content-Type"]) On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Our style guide recommends using single quotes in single-line strings that don't > contain a single quote inside. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:46: def run(server_class=HTTPServer, handler_class=Handler, port=8000, log_format=''): On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Do you really need this function? There's a lot of parameter passing, defaults > and stuff, but unless somebody is going to import this module and call `run()`, > it doesn't seem to be worth it. Maybe just inline its content where it's called > below? Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:53: parser.add_argument('port', action='store', On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > Why is "port" a positional argument while all others are keyword arguments? This > seems a bit arbitrary. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:57: parser.add_argument('--response', action='store', On 2017/08/04 18:01:45, Vasily Kuznetsov wrote: > This is an optional parameter with no default. If it's not supplied, wouldn't > `args.response` be `None`? Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:62: help='Specify the format of the log ouput') On 2017/08/04 18:01:47, Vasily Kuznetsov wrote: > I don't think you need the word "Specify" here. The `help` argument is a > description of the argument, so I don't think it should be an imperative > sentence. For the other arguments you are doing it right IMO. Done. https://codereview.adblockplus.org/29504594/diff/29504595/modules/adblockplus... modules/adblockplus/files/mimeo.py:68: logging.basicConfig(filename=args.log_file, level=logging.INFO) On 2017/08/04 18:01:46, Vasily Kuznetsov wrote: > What do you think about moving this line 2 lines down, so that it's just before > `run(...)`? It would be nicer if all the stuff related to `handler_class` was > together. Done.
Hi Paco, Thanks for addressing the comments. There are a few more things that I've noticed (see below), but mainly, have you tried running this script? It doesn't seem to work for me, at least not unless I very carefully craft the arguments. If you do POSIX-ish arguments parsing and designate some arguments as optional, people will reasonably expect that the script can handle whatever legal combination of arguments is thrown at it. I understand that this is just a piece of infrastructure that will work with specific configuration, and writing full automatic tests for it might be an overkill, but you probably should do some basic testing, at least the no arguments scenario. Another possible option is to remove the expectation that the script can do everything that's implied by its signature (positional and keyword options) and make it take a fixed number of required arguments to handle just this particular use case. Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:15: def get_header_values(self, log_format): What about not passing `log_format`? You can always just access it via `self.log_format`. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:18: name = var.strip() Shouldn't you remove punctuation here instead of spaces? If the variables will look like "$http_var" or "${http_var}", this will not work. Or am I missing something? https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:25: def message_log(self, form, args): This name is somewhat confusing, looks like a method returning a message log, instead of a method that's logging something. Maybe call it 'log_info' instead, if you want to avoid name collision with 'log_message'? https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: 'bytes_sent"': len(content), Oops, double quote :) https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:56: help='Specify alternate port [default: 8000]') I don't think you need "Specify" here either. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:60: parser.add_argument('--log_format', action='store', Probably having some kind of default log format would be good. Does this actually work without a default? https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:64: type=str, nargs='?', default=sys.stdout, Can you pass `sys.stdout` as `filename` to `logging.basicConfig`? For me it just throws an exception.
Thanks for your comments, I addressed the technical ones, and for the arguments, you are right, it makes sense to set sensitive defaults, for the positional one, I did test it but for some reason I thought it worked with stdout as a "file", I think this new iteration handles better the arguments. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:15: def get_header_values(self, log_format): On 2017/08/07 13:06:41, Vasily Kuznetsov wrote: > What about not passing `log_format`? You can always just access it via > `self.log_format`. Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:18: name = var.strip() On 2017/08/07 13:06:41, Vasily Kuznetsov wrote: > Shouldn't you remove punctuation here instead of spaces? If the variables will > look like "$http_var" or "${http_var}", this will not work. Or am I missing > something? Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:25: def message_log(self, form, args): On 2017/08/07 13:06:40, Vasily Kuznetsov wrote: > This name is somewhat confusing, looks like a method returning a message log, > instead of a method that's logging something. Maybe call it 'log_info' instead, > if you want to avoid name collision with 'log_message'? Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: 'bytes_sent"': len(content), On 2017/08/07 13:06:40, Vasily Kuznetsov wrote: > Oops, double quote :) Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:56: help='Specify alternate port [default: 8000]') On 2017/08/07 13:06:40, Vasily Kuznetsov wrote: > I don't think you need "Specify" here either. Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:60: parser.add_argument('--log_format', action='store', On 2017/08/07 13:06:40, Vasily Kuznetsov wrote: > Probably having some kind of default log format would be good. Does this > actually work without a default? Done. https://codereview.adblockplus.org/29504594/diff/29507555/modules/adblockplus... modules/adblockplus/files/mimeo.py:64: type=str, nargs='?', default=sys.stdout, On 2017/08/07 13:06:40, Vasily Kuznetsov wrote: > Can you pass `sys.stdout` as `filename` to `logging.basicConfig`? For me it just > throws an exception. You are right, I got the error as well, I'm pretty sure I tested this...
Hey Paco, Almost there. There are a just couple of comments. Also, I haven't run it, but the defaults for the options now look legit and I assume you did run it. Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:18: name = var.strip(PUNCTUATION) You've added a space to PUNCTUATION above, but do you really need it? Since `var` is coming out of `....split()` above, it should already have no spaces in it, or am I missing something? https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: 'content-type': 'text/plain', Some other variables, like 'time_local' and 'bytes_sent' use underscores and here you use dashes. It's a bit inconsistent and could be surprising for the person who configures the log format. Not a deal-breaker for me, but perhaps you would want them to be consistent. https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:61: default='$remote_addr - - [$time_local] "$request" $status $bytes_sent', This line is too long now, PEP8 (and our style guide) says lines should be <80 characters. Perhaps you can make a constant for the default log format to avoid wrapping lines too much here.
Hey thanks for the quick reply, here it is. :D https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:18: name = var.strip(PUNCTUATION) On 2017/08/07 20:46:01, Vasily Kuznetsov wrote: > You've added a space to PUNCTUATION above, but do you really need it? Since > `var` is coming out of `....split()` above, it should already have no spaces in > it, or am I missing something? Done. https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: 'content-type': 'text/plain', On 2017/08/07 20:46:01, Vasily Kuznetsov wrote: > Some other variables, like 'time_local' and 'bytes_sent' use underscores and > here you use dashes. It's a bit inconsistent and could be surprising for the > person who configures the log format. Not a deal-breaker for me, but perhaps you > would want them to be consistent. Done. https://codereview.adblockplus.org/29504594/diff/29508838/modules/adblockplus... modules/adblockplus/files/mimeo.py:61: default='$remote_addr - - [$time_local] "$request" $status $bytes_sent', On 2017/08/07 20:46:01, Vasily Kuznetsov wrote: > This line is too long now, PEP8 (and our style guide) says lines should be <80 > characters. Perhaps you can make a constant for the default log format to avoid > wrapping lines too much here. Done.
https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:11: proxy_pass http://127.0.0.1:8000; Please explicitly configure the adblockplus::web::mimeo::port above in this file (despite the suitable default), for the sake of being unambiguous and to make sure the configuration is done in the same place as the proxy link. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: def get_header_values(self): Why all these string operations when a simple `re.findall('\\$http_[a-z_0-9]+\\b', self.format)` should do the trick as well? https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:25: def log_info(self, args): While the mimeo output looks like an HTTPd log, it is not logging from the perspective of the mimeo script itself, but regular output. Hence this should not be called log_info, but write_info or something. Analogous it should not use the global resources from module logging, which are for logging from the script perspective, but either a custom logger instance or just regular output. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:48: self.log_request(200) Shouldn't this use the status variable from above instead of the magic number? https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:69: server_class = HTTPServer Why the aliasing, additional variables? https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:70: setattr(handler_class, 'log_format', args.log_format) Analogous to log_info/write_info abobve, this property should be called write_format or just format or similar. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:71: setattr(handler_class, 'response', args.response) Why invocations of setattr() instead of assignments? https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:72: onlyif => 'systemctl status mimeo.service | grep disabled', Why not use `systemctl is-enabled` for this test? https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:86: exec {"reload-daemons": This is a way too global operation to be placed in this manifest, both title- and purpose-wise. Since systemd does not officially support reloading a single daemon, we have only two options here: Use a distinct title and possibly have multiple similar statements throughout all the Puppet setups at some point (possibly invoking daemon-reload multiple times per Puppet run) or introduce and invoke a more central resource. Since this ticket is not about this issue I'd say go for the former and create a follow-up for the latter. See https://tickets.puppetlabs.com/browse/PUP-3483 for more information. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/bin/python3 /opt/mimeo/mimeo.py --response=<%= @response %> --log_format=<%= @format %> --port=<%= @port%> <%= @log_file %> This looks really prone to error due to configuration values that are not aware of the implementation detail that is the lack of quotation here. According to https://github.com/systemd/systemd/issues/624 one can put quotes around the entire argument to avoid the quirky white-space escaping (like you did the accompanying YAML file). Also if you would use separate arguments for option and value, i.e. `--response "example"`, it should be possible to generate the latter via `<%= @response.to_s.to_json %>` -- including the proper quotes and escaping.
https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: def get_header_values(self): On 2017/08/08 07:51:46, mathias wrote: > Why all these string operations when a simple > `re.findall('\\$http_[a-z_0-9]+\\b', self.format)` should do the trick as well? This particular regexp doesn't quite work, I think, because there's also `${http_foo}` syntax, but you could do this with a regexp indeed. It would probably be less code and more readable, so yeah, sounds like a good idea.
On 2017/08/08 11:30:52, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... > File modules/adblockplus/files/mimeo.py (right): > > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... > modules/adblockplus/files/mimeo.py:16: def get_header_values(self): > On 2017/08/08 07:51:46, mathias wrote: > > Why all these string operations when a simple > > `re.findall('\\$http_[a-z_0-9]+\\b', self.format)` should do the trick as > well? > > This particular regexp doesn't quite work, I think, because there's also > `${http_foo}` syntax, but you could do this with a regexp indeed. It would > probably be less code and more readable, so yeah, sounds like a good idea. Not sure if we need to support that syntax. Depends on what the goal is I guess, e.g. "emulate Nginx HTTP log module / log format option" or "recognize Python string template syntax". Either one is fine with me.
On 2017/08/08 11:57:04, mathias wrote: > On 2017/08/08 11:30:52, Vasily Kuznetsov wrote: > > > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... > > File modules/adblockplus/files/mimeo.py (right): > > > > > https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... > > modules/adblockplus/files/mimeo.py:16: def get_header_values(self): > > On 2017/08/08 07:51:46, mathias wrote: > > > Why all these string operations when a simple > > > `re.findall('\\$http_[a-z_0-9]+\\b', self.format)` should do the trick as > > well? > > > > This particular regexp doesn't quite work, I think, because there's also > > `${http_foo}` syntax, but you could do this with a regexp indeed. It would > > probably be less code and more readable, so yeah, sounds like a good idea. > > Not sure if we need to support that syntax. Depends on what the goal is I guess, > e.g. "emulate Nginx HTTP log module / log format option" or "recognize Python > string template syntax". Either one is fine with me. Yay! Me too, as long as it's documented somewhere.
Incoming review https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29508857/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:11: proxy_pass http://127.0.0.1:8000; On 2017/08/08 07:51:45, mathias wrote: > Please explicitly configure the adblockplus::web::mimeo::port above in this file > (despite the suitable default), for the sake of being unambiguous and to make > sure the configuration is done in the same place as the proxy link. Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:16: def get_header_values(self): On 2017/08/08 11:30:51, Vasily Kuznetsov wrote: > On 2017/08/08 07:51:46, mathias wrote: > > Why all these string operations when a simple > > `re.findall('\\$http_[a-z_0-9]+\\b', self.format)` should do the trick as > well? > > This particular regexp doesn't quite work, I think, because there's also > `${http_foo}` syntax, but you could do this with a regexp indeed. It would > probably be less code and more readable, so yeah, sounds like a good idea. Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:25: def log_info(self, args): On 2017/08/08 07:51:46, mathias wrote: > While the mimeo output looks like an HTTPd log, it is not logging from the > perspective of the mimeo script itself, but regular output. Hence this should > not be called log_info, but write_info or something. Analogous it should not use > the global resources from module logging, which are for logging from the script > perspective, but either a custom logger instance or just regular output. Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:48: self.log_request(200) On 2017/08/08 07:51:46, mathias wrote: > Shouldn't this use the status variable from above instead of the magic number? Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:69: server_class = HTTPServer On 2017/08/08 07:51:45, mathias wrote: > Why the aliasing, additional variables? Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:70: setattr(handler_class, 'log_format', args.log_format) On 2017/08/08 07:51:45, mathias wrote: > Analogous to log_info/write_info abobve, this property should be called > write_format or just format or similar. Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:71: setattr(handler_class, 'response', args.response) On 2017/08/08 07:51:45, mathias wrote: > Why invocations of setattr() instead of assignments? The HTTPServer (server_class) receives a server address and a RequestHandlerClass, the Handler is not initialized since it only needs the class not the object, and the `args` are out of the scope of the class, so the only way I found to send the attributes to the class without initializing an object was with setattr https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:72: onlyif => 'systemctl status mimeo.service | grep disabled', On 2017/08/08 07:51:46, mathias wrote: > Why not use `systemctl is-enabled` for this test? Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:86: exec {"reload-daemons": On 2017/08/08 07:51:46, mathias wrote: > This is a way too global operation to be placed in this manifest, both title- > and purpose-wise. Since systemd does not officially support reloading a single > daemon, we have only two options here: Use a distinct title and possibly have > multiple similar statements throughout all the Puppet setups at some point > (possibly invoking daemon-reload multiple times per Puppet run) or introduce and > invoke a more central resource. Since this ticket is not about this issue I'd > say go for the former and create a follow-up for the latter. See > https://tickets.puppetlabs.com/browse/PUP-3483 for more information. Done. https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/bin/python3 /opt/mimeo/mimeo.py --response=<%= @response %> --log_format=<%= @format %> --port=<%= @port%> <%= @log_file %> On 2017/08/08 07:51:46, mathias wrote: > This looks really prone to error due to configuration values that are not aware > of the implementation detail that is the lack of quotation here. According to > https://github.com/systemd/systemd/issues/624 one can put quotes around the > entire argument to avoid the quirky white-space escaping (like you did the > accompanying YAML file). Also if you would use separate arguments for option and > value, i.e. `--response "example"`, it should be possible to generate the latter > via `<%= @response.to_s.to_json %>` -- including the proper quotes and escaping. Jesus, I did look for something like this and was disappointed cause I didn't find anything, thanks!
We're getting there ;) https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:14: LOCK = threading.Lock() Not sure if an upper-case name is appropriate for this one, but either way it should have a prefixing underscore to designate it as an internal thingy. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:22: new_var = name[len(PROXY_TOKEN):].title() Shouldn't this take the `$` into account, because otherwise it is searching for a header that starts with an underscore? And why use method .title() here? I am pretty sure this is not properly recognizing Foo-Bar headers, which should get mapped as $http_foo_bar. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:29: if self.log_file and self.log_file is not sys.stdout: This should probably check for `not in ('/dev/stdout', '-')` or something, not compare a string for identity with a file handle. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:30: fh = open(self.log_file, 'a') Why open and close the file handle with every invocation / request? https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:52: 'content_length': len(content), Since we're emulating Nginx behavior this should actually be prefixed: http://nginx.org/en/docs/http/ngx_http_core_module.html#var_sent_http_ https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:82: setattr(Handler, 'log_file', args.log_file) Again this is not a log, but regular 'output' or a 'target' or, analogous with to method .write_info(), 'info_file', or similar. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:48: file {"/opt/mimeo/mimeo.py": I don't think this requires an entire directory in /opt. And with it, one would expect a bit more than a single file there. The directory is meant for software which does not integrate with the LSB file structure. But we could just copy the script to /usr/local/bin, make it executable and be done with it. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:54: require => File["/opt/mimeo"], I'd consider Package['python3'] a requirement of the script already, not the service resource defined below. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:62: content => template('adblockplus/mimeo.service.erb'), I'd consider the mimeo.py script a requirement of the service unit. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:72: onlyif => 'systemctl is-enabled mimeo.service | grep disabled', You can use parameter $unless and avoid grep by relying on the exit code. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:76: service {"mimeo.service": Just call it Service['mimeo'] please, in Puppet the extension is kind of redundant.
Hi guys, I'm gonna add my 2 Rappen to the Python part of the review. Cheers, Vasily P.S. See https://en.wikipedia.org/wiki/Coins_of_the_Swiss_franc if you don't know what Rappen are. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: REGEX = '\\$' + PROXY_TOKEN + '[a-z_0-9]+\\b' How about making the regex r'\$http_(\w+)\b'? Then you can just grab the first group from the match and you don't need the PROXY_TOKEN constant. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:14: LOCK = threading.Lock() On 2017/08/09 21:09:07, mathias wrote: > Not sure if an upper-case name is appropriate for this one, but either way it > should have a prefixing underscore to designate it as an internal thingy. Yeah, this is not a constant in my book either. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:29: if self.log_file and self.log_file is not sys.stdout: On 2017/08/09 21:09:06, mathias wrote: > This should probably check for `not in ('/dev/stdout', '-')` or something, not > compare a string for identity with a file handle. If it's '/dev/stdout', then you just open it, no? Otherwise we'd have to support all funny ways that OSes have to designate stdout. I agree with the rest of the comment though, it makes sense to support '-' as a file name and just do `fh = sys.stdout` instead of open in this case. Also, the default would be '-' instead of sys.stdout, which is also more appropriate for a command line argument with the type 'str'. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:30: fh = open(self.log_file, 'a') On 2017/08/09 21:09:06, mathias wrote: > Why open and close the file handle with every invocation / request? Just checked, and apparently this class is instantiated for every request. This means we want to create the lock (as we already do) and open/close the file outside of the class.
incoming review https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:14: LOCK = threading.Lock() On 2017/08/09 21:09:07, mathias wrote: > Not sure if an upper-case name is appropriate for this one, but either way it > should have a prefixing underscore to designate it as an internal thingy. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:22: new_var = name[len(PROXY_TOKEN):].title() On 2017/08/09 21:09:06, mathias wrote: > Shouldn't this take the `$` into account, because otherwise it is searching for > a header that starts with an underscore? And why use method .title() here? I am > pretty sure this is not properly recognizing Foo-Bar headers, which should get > mapped as $http_foo_bar. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:29: if self.log_file and self.log_file is not sys.stdout: On 2017/08/10 15:05:57, Vasily Kuznetsov wrote: > On 2017/08/09 21:09:06, mathias wrote: > > This should probably check for `not in ('/dev/stdout', '-')` or something, not > > compare a string for identity with a file handle. > > If it's '/dev/stdout', then you just open it, no? Otherwise we'd have to support > all funny ways that OSes have to designate stdout. > > I agree with the rest of the comment though, it makes sense to support '-' as a > file name and just do `fh = sys.stdout` instead of open in this case. Also, the > default would be '-' instead of sys.stdout, which is also more appropriate for a > command line argument with the type 'str'. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:30: fh = open(self.log_file, 'a') On 2017/08/10 15:05:58, Vasily Kuznetsov wrote: > On 2017/08/09 21:09:06, mathias wrote: > > Why open and close the file handle with every invocation / request? > > Just checked, and apparently this class is instantiated for every request. This > means we want to create the lock (as we already do) and open/close the file > outside of the class. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:52: 'content_length': len(content), On 2017/08/09 21:09:07, mathias wrote: > Since we're emulating Nginx behavior this should actually be prefixed: > http://nginx.org/en/docs/http/ngx_http_core_module.html#var_sent_http_ Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:82: setattr(Handler, 'log_file', args.log_file) On 2017/08/09 21:09:06, mathias wrote: > Again this is not a log, but regular 'output' or a 'target' or, analogous with > to method .write_info(), 'info_file', or similar. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:48: file {"/opt/mimeo/mimeo.py": On 2017/08/09 21:09:07, mathias wrote: > I don't think this requires an entire directory in /opt. And with it, one would > expect a bit more than a single file there. The directory is meant for software > which does not integrate with the LSB file structure. But we could just copy the > script to /usr/local/bin, make it executable and be done with it. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:54: require => File["/opt/mimeo"], On 2017/08/09 21:09:07, mathias wrote: > I'd consider Package['python3'] a requirement of the script already, not the > service resource defined below. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:62: content => template('adblockplus/mimeo.service.erb'), On 2017/08/09 21:09:07, mathias wrote: > I'd consider the mimeo.py script a requirement of the service unit. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:72: onlyif => 'systemctl is-enabled mimeo.service | grep disabled', On 2017/08/09 21:09:07, mathias wrote: > You can use parameter $unless and avoid grep by relying on the exit code. Done. https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:76: service {"mimeo.service": On 2017/08/09 21:09:07, mathias wrote: > Just call it Service['mimeo'] please, in Puppet the extension is kind of > redundant. Done.
Hi Paco! I have some more ideas :) Here we go: https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29510633/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: REGEX = '\\$' + PROXY_TOKEN + '[a-z_0-9]+\\b' On 2017/08/10 15:05:57, Vasily Kuznetsov wrote: > How about making the regex r'\$http_(\w+)\b'? Then you can just grab the first > group from the match and you don't need the PROXY_TOKEN constant. You don't like my idea? Wouldn't the code become more compact and readable? https://codereview.adblockplus.org/29504594/diff/29511665/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29511665/modules/adblockplus... modules/adblockplus/files/mimeo.py:28: message = Template(self.format) You should probably use try/finally here with the locking. Otherwise one error in formatting or writing will bring the whole thing down by not releasing the lock. https://codereview.adblockplus.org/29504594/diff/29511665/modules/adblockplus... modules/adblockplus/files/mimeo.py:66: parser.add_argument('info_file', action='store', I'm not sure if "info_file" is the best name, it sounds a bit unclear. How about "output_file"? https://codereview.adblockplus.org/29504594/diff/29511665/modules/adblockplus... modules/adblockplus/files/mimeo.py:78: setattr(Handler, 'info_file', fh) I think it would be more clear to order the steps like this: # parse args # open the file Handler.format = args.format Handler.response = args.reponse Handler.info_file = fh httpd = HTTPServer(('', args.port), Handler) try: httpd.serve_forever() except: # close the file What do you think?
Hey Paco, Thanks for looking into the comments. Here's some more :) Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29508857/modules/adblockplus... modules/adblockplus/files/mimeo.py:71: setattr(handler_class, 'response', args.response) On 2017/08/09 19:50:48, f.lopez wrote: > On 2017/08/08 07:51:45, mathias wrote: > > Why invocations of setattr() instead of assignments? > > The HTTPServer (server_class) receives a server address and a > RequestHandlerClass, the Handler is not initialized since it only needs the > class not the object, and the `args` are out of the scope of the class, so the > only way I found to send the attributes to the class without initializing an > object was with setattr I'm not sure what you mean here. Setting attributes on classes works fine and produces the same effect as `setattr`. Here's an example from a Python REPL: >>> class A: pass ... >>> A.foo = 'bar' >>> A.foo 'bar' >>> a = A() >>> a.foo 'bar' Or am I still missing something? https://codereview.adblockplus.org/29504594/diff/29514555/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29514555/modules/adblockplus... modules/adblockplus/files/mimeo.py:12: REGEX = '\\$http_[a-z_0-9]+\\b' It's a bit more elegant to use a raw string here: r'\$http_[a-z_0-9]+\b'. Also you can use '\w' special symbol that is equivalent to '[a-zA-Z0-9_]' unless you specifically want to exclude capital letters. https://codereview.adblockplus.org/29504594/diff/29514555/modules/adblockplus... modules/adblockplus/files/mimeo.py:33: sys.stderr.write(e) I've just tested this and it seems to raise a TypeError "write() argument must be str, not Exception". You should probably look into `traceback` module if you want to do this: https://docs.python.org/3/library/traceback.html Speaking about wanting to do this, I hope you realize that you are changing behavior here. Before, if some part of `write_info` would fail, it would cause and error and the user would see an HTTP500 response. Now the exception is swallowed and the user sees a normal response. If that's what you want, all is cool though.
Hi Paco, It seems like the exception handling/logging is still not quite right (see below) + there are a couple of other minor comments. Cheers, Vasily https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:13: REGEX = r'\$http_[\w]+\b' Square brackets are not necessary around '\w', it could be just r'\$http_\w+\b' https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:40: except Exception: You can just write `except` instead of `except Exception`. https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:42: self.send_simple_response(500) When `write_info` returns, the code will try to send response again (in line 58), and that will probably not go well. Perhaps it would be better to move the exception logging/handling to the outer function (do_POST) and only leave try/finally here.
I find counterintuitive to do it separately, I can be wrong but what do you think of my alternative? or something in between. https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:13: REGEX = r'\$http_[\w]+\b' On 2017/08/15 18:56:25, Vasily Kuznetsov wrote: > Square brackets are not necessary around '\w', it could be just r'\$http_\w+\b' Done. https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:40: except Exception: On 2017/08/15 18:56:25, Vasily Kuznetsov wrote: > You can just write `except` instead of `except Exception`. Done. https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... modules/adblockplus/files/mimeo.py:42: self.send_simple_response(500) On 2017/08/15 18:56:25, Vasily Kuznetsov wrote: > When `write_info` returns, the code will try to send response again (in line > 58), and that will probably not go well. Perhaps it would be better to move the > exception logging/handling to the outer function (do_POST) and only leave > try/finally here. What if I use this function to return the code instead? if it catches an exception I send back a 500 and if not I send back a 200?, that way I can actually have more control over the main functionality of the script, so for different errors/conditionals I can even return different status codes? And then in the line 58 I just send the `status` I got from the function?
https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29515603/modules/adblockplus... 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 18:56:25, Vasily Kuznetsov wrote: > > When `write_info` returns, the code will try to send response again (in line > > 58), and that will probably not go well. Perhaps it would be better to move > the > > exception logging/handling to the outer function (do_POST) and only leave > > try/finally here. > > What if I use this function to return the code instead? > > if it catches an exception I send back a 500 and if not I send back a 200?, that > way I can actually have more control over the main functionality of the script, > so for different errors/conditionals I can even return different status codes? > > And then in the line 58 I just send the `status` I got from the function? Yes, this approach can work too, although you would also need to deal with the different content in 200 and 500 responses. Perhaps `write_info` could return `True/False` depending on whether it managed to write things out successfully. You'd end up with something like this: def write_info(self, args): _lock.acquire() try: # ... except: traceback.print_exc(file=sys.stderr) return False finally: _lock.release() return True def do_POST(self): # ... if self.write_info(values): self.send_simple_response(status, content) else: self.send_simple_response(500) However, I think the code would be simpler and have less branching if you just move all exception handling to the outside, like this: def write_info(self, args): _lock.acquire() try: # ... finally: _lock.release() def do_POST(self): # ... try: self.write_info(values) self.send_simple_response(status, content) except: traceback.print_exc(file=sys.stderr) self.send_simple_response(500) With this approach, you can also use the fact that locks support context manager protocol [1] and rewrite `write_info` using `with` for additional style points: def write_info(self, args): with _lock: # ... [1]: https://docs.python.org/3.6/library/threading.html#with-locks
Hi Paco, Assuming you've tested this code, LGTM. Cheers, Vasily
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:24: values[name[1:]] = self.headers.get(new_var, '-') Does this understand translation between i.e. `X-Forwarded-For` and `x_forwarded_for`? https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: self.output.write(message.safe_substitute(args) + '\n') Shouldn't the template stuff happen before, so that the lock would only be held for writing a string and flushing? https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:79: fh = sys.stdout If you would `os.open(sys.stdout.fileno(), 'w', 0)` you could avoid conditionals like the one around `fh.close()` later on. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:85: try: This exception handling should enclose everything since the file has been opened, at least everything that is not an assignment and can possibly fail (e.g. the HTTPServer instantiation above). https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:4: # machine. Mimeo is used here since it is the abbreviation and it sounds better. Please don't explain naming unless absolutely necessary. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:6: # The pourpose of this class is to log the information received in a http/s Please name the item the fuzz is about and avoid non-content, i.e. "Class foobar maintains a ..." vs "The purpose of this class is to maintain a ...". https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:8: # === Parameters: There should be a `#`-only line above this one. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:25: class adblockplus::web::mimeo ( There should be a `#`-only line above this one as well. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: $log_file = '/var/log/mimeo.log', Again this is not logging in the word's sense within our technical language domain, and it was merely a coincidence that we've been using logging mechanisms for the tasks we now want to do with the `mimeo.py` script. So please use some terminology that avoids the confusion, i.e. `output` or `data` or similar, and use some path in `/var/adblockplus/mimeo/` or similar. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:39: content => file('adblockplus/mimeo.py'), Why use $content/file() here instead of $source? https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:69: Package["python3"], The service should be able to rely on the script managing it's own dependencies, so there should be no need to declare a relationship with Python here. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:74: command => 'systemctl daemon-reload', Does daemon-reload include a service restart if necessary? Not sure but I don't think so, in which case changing the mimeo.service file would not currently result in a restart. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/local/bin/mimeo.py '--response=<%= @response.to_s.to_json %>' '--format=<%= @format %>' '--port=<%= @port%>' <%= @log_file %> This is not what we discussed and a bad copy of my suggestion. Instead of `--response <%= @reponse.to_s.to_json %>` (two separate parameters where the 2nd takes advantage of JSON escaping and quotation) you now use a construct that results in the output message not being `response` but `"response"` with quotes (or am I reading that wrong?). The rationale applies to all parameters, hence it should be `--format <%= @format.to_s.to_json %>` and `<%= @log_file.to_s.to_json %>` (how about renaming that one to also not use `log_`, btw?). The `--port=<% @port.to_i %>` could be given as one, though for the sake of consistency it should be two separate arguments as well - IF you agree with my suggestions.
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:79: fh = sys.stdout On 2017/08/18 08:47:43, mathias wrote: > If you would `os.open(sys.stdout.fileno(), 'w', 0)` you could avoid conditionals > like the one around `fh.close()` later on. This doesn't work (at least in Python 3.6), but what you can indeed do is `open(sys.stdout.fileno(), 'w', closefd=False)` using builtin open, not the one from os module. This will give you another file-like object that will use the same file descriptor as `sys.stdout` under the hood but will not close it when its closed. This way you can indeed eliminate the closing `if` that Matze doesn't like.
On 2017/08/18 12:49:58, Vasily Kuznetsov wrote: > https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... > File modules/adblockplus/files/mimeo.py (right): > > https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... > modules/adblockplus/files/mimeo.py:79: fh = sys.stdout > On 2017/08/18 08:47:43, mathias wrote: > > If you would `os.open(sys.stdout.fileno(), 'w', 0)` you could avoid > conditionals > > like the one around `fh.close()` later on. > > This doesn't work (at least in Python 3.6), but what you can indeed do is > `open(sys.stdout.fileno(), 'w', closefd=False)` using builtin open, not the one > from os module. This will give you another file-like object that will use the > same file descriptor as `sys.stdout` under the hood but will not close it when > its closed. This way you can indeed eliminate the closing `if` that Matze > doesn't like. Yeah my code wasn't tested, sorry, so please try Vasily's version. Though I am a bit skeptic about the default buffering behavior here, not sure if that is the same. (I believe I confused my example with os.fdopen(), which I've used with a 3rd parameter for buffering before, but that is the same as open() in Python 3 now.)
I'm sending a new review and wait for your comments on how we should handle the headers. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:24: values[name[1:]] = self.headers.get(new_var, '-') On 2017/08/18 08:47:43, mathias wrote: > Does this understand translation between i.e. `X-Forwarded-For` and > `x_forwarded_for`? No, one has to match them manually. proxy_set_header X-Forwarded-For $http_x_forwarded_for; Other option could be to use a list/tuple/something to match all cases? ('x_forwarded_for', X_Forwarded_for', 'X-Forwarded-For') Or set a standard to for how the headers should look like. What do you think? https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: self.output.write(message.safe_substitute(args) + '\n') On 2017/08/18 08:47:44, mathias wrote: > Shouldn't the template stuff happen before, so that the lock would only be held > for writing a string and flushing? Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:79: fh = sys.stdout On 2017/08/18 12:49:57, Vasily Kuznetsov wrote: > On 2017/08/18 08:47:43, mathias wrote: > > If you would `os.open(sys.stdout.fileno(), 'w', 0)` you could avoid > conditionals > > like the one around `fh.close()` later on. > > This doesn't work (at least in Python 3.6), but what you can indeed do is > `open(sys.stdout.fileno(), 'w', closefd=False)` using builtin open, not the one > from os module. This will give you another file-like object that will use the > same file descriptor as `sys.stdout` under the hood but will not close it when > its closed. This way you can indeed eliminate the closing `if` that Matze > doesn't like. Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:85: try: On 2017/08/18 08:47:44, mathias wrote: > This exception handling should enclose everything since the file has been > opened, at least everything that is not an assignment and can possibly fail > (e.g. the HTTPServer instantiation above). Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:4: # machine. Mimeo is used here since it is the abbreviation and it sounds better. On 2017/08/18 08:47:44, mathias wrote: > Please don't explain naming unless absolutely necessary. Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:6: # The pourpose of this class is to log the information received in a http/s On 2017/08/18 08:47:45, mathias wrote: > Please name the item the fuzz is about and avoid non-content, i.e. "Class foobar > maintains a ..." vs "The purpose of this class is to maintain a ...". Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:8: # === Parameters: On 2017/08/18 08:47:46, mathias wrote: > There should be a `#`-only line above this one. Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:25: class adblockplus::web::mimeo ( On 2017/08/18 08:47:45, mathias wrote: > There should be a `#`-only line above this one as well. Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: $log_file = '/var/log/mimeo.log', On 2017/08/18 08:47:44, mathias wrote: > Again this is not logging in the word's sense within our technical language > domain, and it was merely a coincidence that we've been using logging mechanisms > for the tasks we now want to do with the `mimeo.py` script. So please use some > terminology that avoids the confusion, i.e. `output` or `data` or similar, and > use some path in `/var/adblockplus/mimeo/` or similar. I'll just default it to '-' (stdout) https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:69: Package["python3"], On 2017/08/18 08:47:46, mathias wrote: > The service should be able to rely on the script managing it's own dependencies, > so there should be no need to declare a relationship with Python here. Done. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:74: command => 'systemctl daemon-reload', On 2017/08/18 08:47:45, mathias wrote: > Does daemon-reload include a service restart if necessary? Not sure but I don't > think so, in which case changing the mimeo.service file would not currently > result in a restart. Heh, you are right, I forgot to notify the service. https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/local/bin/mimeo.py '--response=<%= @response.to_s.to_json %>' '--format=<%= @format %>' '--port=<%= @port%>' <%= @log_file %> On 2017/08/18 08:47:47, mathias wrote: > This is not what we discussed and a bad copy of my suggestion. Instead of > `--response <%= @reponse.to_s.to_json %>` (two separate parameters where the 2nd > takes advantage of JSON escaping and quotation) you now use a construct that > results in the output message not being `response` but `"response"` with quotes > (or am I reading that wrong?). > > The rationale applies to all parameters, hence it should be `--format <%= > @format.to_s.to_json %>` and `<%= @log_file.to_s.to_json %>` (how about renaming > that one to also not use `log_`, btw?). The `--port=<% @port.to_i %>` could be > given as one, though for the sake of consistency it should be two separate > arguments as well - IF you agree with my suggestions. Well what you suggested is documented and it's supposed to work, but I tried different ways and I couldn't make it work with the C-style escape. The documentation says "C-style escapes are also supported" so I just replaced the backquotes for normal single quotes, also the docs says "the opening quote may appear only after whitespace, and the closing quote must be followed by whitespace or the end of line" which made me use the "=" (equal) character in order to avoid whitespaces in between, which resulted in this version. Am I missing something? I tried several options using different escape mechanisms and this one is the one I made it work every time reliable.
https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/files/mimeo.py:24: values[name[1:]] = self.headers.get(new_var, '-') On 2017/08/19 00:51:00, f.lopez wrote: > On 2017/08/18 08:47:43, mathias wrote: > > Does this understand translation between i.e. `X-Forwarded-For` and > > `x_forwarded_for`? > > No, one has to match them manually. > proxy_set_header X-Forwarded-For $http_x_forwarded_for; > > Other option could be to use a list/tuple/something to match all cases? > ('x_forwarded_for', X_Forwarded_for', 'X-Forwarded-For') > > Or set a standard to for how the headers should look like. > > What do you think? No idea what you mean. I was referring to this code passing on lowercase_and_underscore_separated_header_names were one would expect Titlelized-And-Minus-Separated-Header-Names and asking if self.headers.get() can indeed handle that or if you need to string-transform your input first. I would guess the latter, but only replacing underscores with minuses (the starting letters should be examined in case-insensitive fashion anyway, if you're lucky that is). https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29517738/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/local/bin/mimeo.py '--response=<%= @response.to_s.to_json %>' '--format=<%= @format %>' '--port=<%= @port%>' <%= @log_file %> On 2017/08/19 00:51:02, f.lopez wrote: > On 2017/08/18 08:47:47, mathias wrote: > > This is not what we discussed and a bad copy of my suggestion. Instead of > > `--response <%= @reponse.to_s.to_json %>` (two separate parameters where the > 2nd > > takes advantage of JSON escaping and quotation) you now use a construct that > > results in the output message not being `response` but `"response"` with > quotes > > (or am I reading that wrong?). > > > > The rationale applies to all parameters, hence it should be `--format <%= > > @format.to_s.to_json %>` and `<%= @log_file.to_s.to_json %>` (how about > renaming > > that one to also not use `log_`, btw?). The `--port=<% @port.to_i %>` could be > > given as one, though for the sake of consistency it should be two separate > > arguments as well - IF you agree with my suggestions. > > Well what you suggested is documented and it's supposed to work, but I tried > different ways and I couldn't make it work with the C-style escape. > The documentation says "C-style escapes are also supported" so I just replaced > the backquotes for normal single quotes, also the docs says "the opening quote > may appear only after whitespace, and the closing quote must be followed by > whitespace or the end of line" which made me use the "=" (equal) character in > order to avoid whitespaces in between, which resulted in this version. > > Am I missing something? I tried several options using different escape > mechanisms and this one is the one I made it work every time reliable. I have no idea what you mean. I did not mention anything C-style. Try reading my comment again, but note that a) BACKTICKS (`) enclose code-snippets in my examples, but aren't part of the actual code, b) I did explicitly refer to two separate parameters and NOT a single one where key and value are separated by an equals sign (=), and c) there is a difference between quoting and escaping, e.g. there's no such thing as a BACKQUOTE. https://codereview.adblockplus.org/29504594/diff/29519675/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29519675/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: self.output.write(message.safe_substitute(args) + '\n') The substitution and concatenation should also happen outside the lock.
Ok I get it now, so when you suggested the backticks (not backquotes, my bad) I misunderstood it and I was actually using them as part of the code... That said, I think it's better to use the quotes around the whole argument `'--format=<%= format %>'` since the system will escape the string. I'll send a new codereview now. https://codereview.adblockplus.org/29504594/diff/29519675/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29519675/modules/adblockplus... modules/adblockplus/files/mimeo.py:37: self.output.write(message.safe_substitute(args) + '\n') On 2017/08/19 15:06:08, mathias wrote: > The substitution and concatenation should also happen outside the lock. Done.
Getting closer.. :) https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/files/mimeo.py:87: except: Shouldn't this be `finally`? https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: $output = '-', With that one being the default now, how exactly can one access the data in development? https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:75: File["/usr/local/bin/mimeo.py"], Why should changing the Python script require a daemon-reload? IMHO the Exec['reload-mimeo-daemon'] should subscribe to File['/etc/systemd/system/mimeo.service'], and Service['mimeo'] should subscribe to File['/usr/local/bin/mimeo.py']. https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:2: Description=Service that handles formating and parsing of logs Does it?
https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/files/mimeo.py:87: except: On 2017/08/21 07:37:42, mathias wrote: > Shouldn't this be `finally`? Done. https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: $output = '-', On 2017/08/21 07:37:42, mathias wrote: > With that one being the default now, how exactly can one access the data in > development? Done. https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:75: File["/usr/local/bin/mimeo.py"], On 2017/08/21 07:37:42, mathias wrote: > Why should changing the Python script require a daemon-reload? IMHO the > Exec['reload-mimeo-daemon'] should subscribe to > File['/etc/systemd/system/mimeo.service'], and Service['mimeo'] should subscribe > to File['/usr/local/bin/mimeo.py']. Done. https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29521555/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:2: Description=Service that handles formating and parsing of logs On 2017/08/21 07:37:43, mathias wrote: > Does it? Done.
Almost! https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:20: mimeo_data: Sorry, but since it's hard-coded in the manifest now we need to integrate it there as well. Leave it out here, we can do that in a follow-up. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/files/mimeo.py:75: try: The `try` block should not start before `fh` is defined. Otherwise you may trigger an Exception in the `finally` block, e.g. when `parser.parse_args()` raises an exception. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: realize(File['/var/adblockplus']) Please explicitly include `adblockplus` when you realize its resources. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:46: File['/var/adblockplus/mimeo'], The script does not depend on the path hard-coded in the service definition. The service definition does. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/local/bin/mimeo.py '--response=<%= @response %>' '--format=<%= @format %>' '--port=<%= @port %>' /var/adblockplus/mimeo/mimeo.data Just call it `data` or use another character (`-` or `_` or something), no need to (re-) invent some file extension.
Incoming review https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adb... File hiera/roles/web/adblockbrowser.yaml (right): https://codereview.adblockplus.org/29504594/diff/29522661/hiera/roles/web/adb... hiera/roles/web/adblockbrowser.yaml:20: mimeo_data: On 2017/08/21 20:07:38, mathias wrote: > Sorry, but since it's hard-coded in the manifest now we need to integrate it > there as well. Leave it out here, we can do that in a follow-up. Acknowledged. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/files/mimeo.py (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/files/mimeo.py:75: try: On 2017/08/21 20:07:38, mathias wrote: > The `try` block should not start before `fh` is defined. Otherwise you may > trigger an Exception in the `finally` block, e.g. when `parser.parse_args()` > raises an exception. Done. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:29: realize(File['/var/adblockplus']) On 2017/08/21 20:07:38, mathias wrote: > Please explicitly include `adblockplus` when you realize its resources. Done. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:46: File['/var/adblockplus/mimeo'], On 2017/08/21 20:07:38, mathias wrote: > The script does not depend on the path hard-coded in the service definition. The > service definition does. Yeah, you are right, I wasn't sure about this tbh. https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... File modules/adblockplus/templates/mimeo.service.erb (right): https://codereview.adblockplus.org/29504594/diff/29522661/modules/adblockplus... modules/adblockplus/templates/mimeo.service.erb:5: ExecStart=/usr/local/bin/mimeo.py '--response=<%= @response %>' '--format=<%= @format %>' '--port=<%= @port %>' /var/adblockplus/mimeo/mimeo.data On 2017/08/21 20:07:39, mathias wrote: > Just call it `data` or use another character (`-` or `_` or something), no need > to (re-) invent some file extension. Done.
https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus... File modules/adblockplus/manifests/web/mimeo.pp (right): https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus... modules/adblockplus/manifests/web/mimeo.pp:74: File['/var/adblockplus/mimeo'], The directory is an implicit dependency (via mimeo.py), not an explicit one, so no need for this item.
On 2017/08/22 17:30:58, mathias wrote: > https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus... > File modules/adblockplus/manifests/web/mimeo.pp (right): > > https://codereview.adblockplus.org/29504594/diff/29523695/modules/adblockplus... > modules/adblockplus/manifests/web/mimeo.pp:74: File['/var/adblockplus/mimeo'], > The directory is an implicit dependency (via mimeo.py), not an explicit one, so > no need for this item. Actually that's bullshit, it is an implicit dependency, but not via mimeo.py -- obviously.
LGTM.
Also LGTM for 'mimeo.py' in patch set 14. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
