Summary

Support running tools manually

Review Request #10316 — Created Nov. 8, 2018 and discarded Aug. 10, 2020, 3:40 p.m.

Information

Owner

alextechcc

Repository

ReviewBot

Branch

master

Bugs

Depends On

~~10311~~

Reviewers

Groups

reviewbot, students

People

Description

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.

Testing Done

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

Issues

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 …	brennie	Nov. 29, 2018, 8:04 p.m.
Looks good to me and reads well.	gojeffcho	Dec. 6, 2018, 5:22 p.m.
This needs a a help_text= kwarg to describe what exactly this means. Maybe we should call this manually_run_only and change …	brennie	Nov. 15, 2018, 6:58 p.m.
This should go at the top of the function and there should be a blank line after.	brennie	Nov. 15, 2018, 7:22 p.m.
E303 too many blank lines (2)	reviewbot	Nov. 26, 2018, 8:27 p.m.
E303 too many blank lines (2)	reviewbot	Nov. 26, 2018, 8:27 p.m.
Can we call this _get_maching_configs ? We call them configurations in the docstring.	brennie	Nov. 29, 2018, 7:44 p.m.
Format as: Yields: tuple: A 3-tuple of the following: * The tool model to use (:py:class:`reviewbotext.models.Tool`). * The tool options …	brennie	Nov. 29, 2018, 7:56 p.m.
Can you format as: for tool, tool_options, review_settings in self._get_matching_setups( review_request): # ... You can also pull out the function …	brennie	Nov. 29, 2018, 8:03 p.m.
No blank line.	brennie	Nov. 29, 2018, 8:04 p.m.
Same comment as above.	brennie	Nov. 29, 2018, 8:04 p.m.
Let's put this at the end of the tuple. I expect it to be used less often than the other …	david	Dec. 6, 2018, 3:49 p.m.
Lets just say "configurations" instead of "tool, review configs"	brennie	Dec. 19, 2018, 11:22 a.m.
All the other descriptions are lower case. Do we want this to be too? Also, shouldn't this (and all other …	brennie	Dec. 19, 2018, 11:22 a.m.
same here re: translation. If we end up using translated descriptions and we are using them in multiple places, they …	brennie	Dec. 19, 2018, 11:22 a.m.

flake8 passed.

JSHint passed.

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?

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.

Commit:

-	11e5205ffbcb711b0e4669b988dfbec3d434fc9e
+	b167436389cf745d4b5f7de5a796461bda7cbd21

Diff:

Revision 2 (+137 -11)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

extension/reviewbotext/integration.py (Diff revision 2)
Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

Update timestamp when running/rerunning tool

Commit:

-	b167436389cf745d4b5f7de5a796461bda7cbd21
+	a75bcfd93b5ccb2480bc371f95b5fb1284063a01

Diff:

Revision 3 (+139 -11)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

extension/reviewbotext/integration.py (Diff revision 3)
Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

Removed WIP tag, attmpted to clean up duplicate code with a new function that generates valid setups.

Summary:

-	[WIP] Initial addition of ReviewBot manual run feature
+	Initial addition of ReviewBot manual run feature

Description:

~		Initial addition of manual run feature
	~	Added manual run option to integration configurations, which allows for
	+	manual run instead of tools running on review request updates.

~		Not looking for feedback yet.
~		(aside from high-level design direction)
	~	Added new signal hook for manual run feature that will update previous
	~	status updates that have not been run yet or need a rerun.

~		Duplicate code will be cleaned up.
	~	Cleaned up code by adding a generator for matching tool setups.
	+
	+	Some improvements could be made to the `_get_matching_setups()` function.

Testing Done:

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

Commit:

-	a75bcfd93b5ccb2480bc371f95b5fb1284063a01
+	48ac68c658b71538fcb035bd67c873ce33b55d61

Diff:

Revision 4 (+155 -49)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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"

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

Show all issues

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

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?

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:
    # ...

extension/reviewbotext/integration.py (Diff revision 4)
Show all issues
```
No blank line.
```
extension/reviewbotext/integration.py (Diff revision 4)
Show all issues
```
Same comment as above.
```

Change Summary:

Updated description and documentation wording, moved missing configuration check into main code, minor formatting changes.

Description:

~		Added manual run option to integration configurations, which allows for
~		manual run instead of tools running on review request updates.
	~	Support running tools attached to status updates manually, along with
	~	retrying status updates in an error state.

~		Added new signal hook for manual run feature that will update previous
~		status updates that have not been run yet or need a rerun.
	~	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.
-
-		Cleaned up code by adding a generator for matching tool setups.
-
-		Some improvements could be made to the `_get_matching_setups()` function.

Commit:

-	48ac68c658b71538fcb035bd67c873ce33b55d61
+	268ebe6c0fcd791a99bec563048b53d726bb7749

Diff:

Revision 5 (+165 -47)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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.

Summary:

-	Initial addition of ReviewBot manual run feature
+	Support running tools manually

Change Summary:

Reordered tool options in form

Commit:

-	268ebe6c0fcd791a99bec563048b53d726bb7749
+	0f7a3d936a3cf162b5151002ac51003eed451ca0

Diff:

Revision 6 (+165 -47)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues
```
Looks good to me and reads well.
```

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

Show all issues

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

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?

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

Change Summary:

Made descriptions translatable, used constants to maintain consistency, fixed capitalization.

Commit:

-	0f7a3d936a3cf162b5151002ac51003eed451ca0
+	ad1bddbdf37384d2c250f29474ee98376479ee2d

Diff:

Revision 7 (+168 -47)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Mirroring changes in other review request

Commit:

-	ad1bddbdf37384d2c250f29474ee98376479ee2d
+	e9a741274e189c8f4dc18fae4934e2a46a7c11ae

Diff:

Revision 8 (+168 -47)

Show changes

	extension/reviewbotext/forms.py
	extension/reviewbotext/integration.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ping... is this still in progress? I really like to see this.

Status: Discarded

Change Summary:

Supplanted by /r/11130/