Add manual run and retry for Review Bot tools.

Review Request #11130 — Created Aug. 10, 2020 and submitted

david
ReviewBot
master
4649
154f645...
reviewbot

This change adds the ability for Review Bot to run tools manually. This
is used in two ways:

  • Tool configurations can now be set to only be run manually. In this
    case, the tool will create a status update in the NOT_YET_RUN state.
  • All tool configurations will mark the status update as retryable,
    which will allow the trigger of a retry in the case of error or
    timeout states.

Based on work by Alex Klemenchuk at /r/10316

  • Created a configuration set to only run manually. Verified that the
    initial status update was correctly created in the NOT_YET_RUN state,
    and that clicking the "Run" button triggered a tool run and subsequent
    update.
  • Triggered the run of a tool not normally set to run manually but
    without running the worker process (resulting in a timeout). Saw that
    the "Retry" button was shown and that clicking it re-ran the tool.
  • Tested that the correct diffset was chosen when running and retrying
    tools.
Description From Last Updated

This needs some work to gate off the Run Manually capabilities if not present in Review Board. We don't want ...

chipx86chipx86

_get_matching_configs() is a generator, so the if check and len and [0] below don't seem like they'd work?

chipx86chipx86

It's apparent in retrospect, but (and this is probably me being tired) it took too long to realize why we ...

chipx86chipx86

Probably worth tracking this in Asana with some info on what's wrong.

chipx86chipx86

Maybe worth doing this filtering in _get_matching_configs so we don't compute any more data than we have to?

chipx86chipx86

Got a bunch of thoughts here. It seems we can get, say, 15 configs here. So: Each time we get ...

chipx86chipx86

Can we use update_fields= here?

chipx86chipx86
chipx86
  1. 
      
  2. This needs some work to gate off the Run Manually capabilities if not present in Review Board. We don't want to outright crash on 3.0.18 or older.

  3. 
      
david
chipx86
  1. 
      
  2. extension/reviewbotext/integration.py (Diff revision 2)
     
     
     

    It's apparent in retrospect, but (and this is probably me being tired) it took too long to realize why we were checking this and bailing. Can we add a comment saying that we're skipping status updates performed by other scripts or extensions?

  3. extension/reviewbotext/integration.py (Diff revision 2)
     
     
     

    Probably worth tracking this in Asana with some info on what's wrong.

  4. extension/reviewbotext/integration.py (Diff revision 2)
     
     
     
     
     

    Maybe worth doing this filtering in _get_matching_configs so we don't compute any more data than we have to?

  5. extension/reviewbotext/integration.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Got a bunch of thoughts here.

    It seems we can get, say, 15 configs here. So:

    1. Each time we get one, we're setting some of the same state on status_update and then saving it, which ideally we wouldn't do (too many hits to the database, unnecessary timestamp updates).
    2. We're also fetching the diff each time. This is going to be the same diff every time, but we're pulling from the database each time.
    3. We're also setting up a session using extension.login_user() above, even if we have zero matching configs returned.

    I might be wrong, but I think a better approach would be to have _get_matching_configs() to do the filtering (as mentioned in a prior comment) and then:

    if matching_configs:
        repository = ...
        changedesc = ...
    
        # Fetch the diffset here.
        try:
            ...
        except DiffSet.DoesNotExist:
            ...
            return
    
        session = extension.login_user()
        ...
    
        status_update.description = ...
        status_update.state = ...
        status_update.timestamp = ...
        status_update.save(update_fields=('description', 'state', 'timestamp'))
    
        for ... in matching_configs:
            queue = ...
    
            extension.celery.send_task(...)
    
    1. I'll definitely make it more clear, but fundamentally this isn't an issue here because this is coming from the request-run signal, so there will only be a single config out of the set of matching configs that also matches the service_id check. I'll change it to pull out the single matching configuration so that it's clear it's only (at most) one.

  6. extension/reviewbotext/integration.py (Diff revision 2)
     
     

    Can we use update_fields= here?

  7. 
      
david
chipx86
  1. 
      
  2. extension/reviewbotext/integration.py (Diff revisions 2 - 3)
     
     
     
     
     

    _get_matching_configs() is a generator, so the if check and len and [0] below don't seem like they'd work?

  3. 
      
david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (4154985)
Loading...