Support running tools manually

Review Request #10316 — Created Nov. 8, 2018 and discarded

Information

ReviewBot
master

Reviewers

Support running tools attached to status updates manually, along with
retrying status updates in an error state.

Adds a new signal hook for the manual run feature that will update
previous status updates that have not been run yet or need a rerun.

Manual testing has been done (waiting on testing framework). By adding
multiple integrations for different types of bots, running some
manually, and retrying some.

Description From Last Updated

Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the …

brenniebrennie

Looks good to me and reads well.

gojeffchogojeffcho

This needs a a help_text= kwarg to describe what exactly this means. Maybe we should call this manually_run_only and change …

brenniebrennie

This should go at the top of the function and there should be a blank line after.

brenniebrennie

E303 too many blank lines (2)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Can we call this _get_maching_configs ? We call them configurations in the docstring.

brenniebrennie

Format as: Yields: tuple: A 3-tuple of the following: * The tool model to use (:py:class:`reviewbotext.models.Tool`). * The tool options …

brenniebrennie

Can you format as: for tool, tool_options, review_settings in self._get_matching_setups( review_request): # ... You can also pull out the function …

brenniebrennie

No blank line.

brenniebrennie

Same comment as above.

brenniebrennie

Let's put this at the end of the tuple. I expect it to be used less often than the other …

daviddavid

Lets just say "configurations" instead of "tool, review configs"

brenniebrennie

All the other descriptions are lower case. Do we want this to be too? Also, shouldn't this (and all other …

brenniebrennie

same here re: translation. If we end up using translated descriptions and we are using them in multiple places, they …

brenniebrennie
brennie
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 1)
     
     
    Show all issues

    This needs a a help_text= kwarg to describe what exactly this means.

    Maybe we should call this manually_run_only and change the label to "Only run this tool manually" or similar?

  3. extension/reviewbotext/integration.py (Diff revision 1)
     
     
    Show all issues

    This should go at the top of the function and there should be a blank line after.

  4. 
      
alextechcc
Review request changed
Commit:
11e5205ffbcb711b0e4669b988dfbec3d434fc9e
b167436389cf745d4b5f7de5a796461bda7cbd21

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
Review request changed
Change Summary:

Update timestamp when running/rerunning tool

Commit:
b167436389cf745d4b5f7de5a796461bda7cbd21
a75bcfd93b5ccb2480bc371f95b5fb1284063a01

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

alextechcc
brennie
  1. 
      
  2. Show all issues

    Can you rewrite your summary in the imperitive mood, i.e. as a command? If you substitute your summary into the following template, it should be a correct sentence.

    This patch will <summary>
    

    e.g. "Support running tools manually"

  3. extension/reviewbotext/integration.py (Diff revision 4)
     
     
    Show all issues

    Can we call this _get_maching_configs ? We call them configurations in the docstring.

  4. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     
     
     
     
     
    Show all issues

    Format as:

    Yields:
        tuple:
        A 3-tuple of the following:
    
        * The tool model to use (:py:class:`reviewbotext.models.Tool`).
        * The tool options (:py:class:`dict`).
        * The review settings (:py:class:`dict`).
    

    Could you also elaborate on what review_settings actually is in that?

  5. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     
    Show all issues

    Can you format as:

    for tool, tool_options, review_settings in self._get_matching_setups(
        review_request):
        # ...
    

    You can also pull out the function call into a var and iterate over that, e.g.

    setups = self._get_matching_setups(review_request)
    
    for a, b, c in setups:
        # ...
    
  6. extension/reviewbotext/integration.py (Diff revision 4)
     
     
    Show all issues

    No blank line.

  7. extension/reviewbotext/integration.py (Diff revision 4)
     
     
     
    Show all issues

    Same comment as above.

  8. 
      
alextechcc
david
  1. 
      
  2. extension/reviewbotext/forms.py (Diff revision 5)
     
     
    Show all issues

    Let's put this at the end of the tuple. I expect it to be used less often than the other options.

  3. 
      
alextechcc
alextechcc
gojeffcho
  1. 
      
  2. Show all issues

    Looks good to me and reads well.

  3. 
      
brennie
  1. 
      
  2. extension/reviewbotext/integration.py (Diff revision 6)
     
     
    Show all issues

    Lets just say "configurations" instead of "tool, review configs"

  3. extension/reviewbotext/integration.py (Diff revision 6)
     
     
    Show all issues

    All the other descriptions are lower case. Do we want this to be too?

    Also, shouldn't this (and all other descriptions) be marked for translation?

  4. extension/reviewbotext/integration.py (Diff revision 6)
     
     
    Show all issues

    same here re: translation.

    If we end up using translated descriptions and we are using them in multiple places, they should be constants somewhere so they stay in sync with one another

  5. 
      
alextechcc
alextechcc
misery
  1. 
      
  2. Ping... is this still in progress? I really like to see this.

  3. 
      
david
Review request changed
Status:
Discarded
Change Summary:

Supplanted by /r/11130/