Summary

Add status-update command to rbtools.

Review Request #9343 — Created Nov. 1, 2017 and submitted June 4, 2018, 2:28 p.m.

Information

Owner

bleblan2

Repository

RBTools

Branch

release-0.7.x

Bugs

Depends On

Commit

dd36fbb...

Reviewers

Groups

rbtools, students

People

Description

Review Board 3.0 introduced new support for "status updates" which are
used to indicate if a review request is undergoing some automated checks
of some kind (for instance, automated code review or pending builds).
These can have a state (such as "pending", "error", "success", and
others), a summary, optional link, and a review (when there are errors).

This adds a new command for working with status updates, helping to
create/update the state of a status update. This command can be run with
rbt status-update.

Testing Done

Debug testing of the --review input on local installation with the following file contents:

{
    "review": {
        "body_top": "Header comment"
    },
    "diff_comment": [
        {
            "filediff_id": 10,
            "first_line": 729,
            "issue_opened": true,
            "num_lines": 1,
            "text": "Adding a comment on a diff line",
            "text_type": "markdown"
        }
    ],
    "general_comments": [
        {
            "text": "Adding a general comment",
            "text_type": "markdown"
        }
    ]
}

Issues

Description	From	Last Updated
Missing docs rst file for new status-update command.	bleblan2	Dec. 10, 2017, 3:05 p.m.
You might want to pull out each action into its own function.	brennie	Nov. 1, 2017, 5:12 p.m.
I know most files in rbtools don't have them (yet), but we should have a file docstring at the top.	david	Dec. 5, 2017, 9:47 a.m.
Add a blank line between these two.	david	Dec. 5, 2017, 9:47 a.m.
Needs "Args" and "Returns" sections.	david	Dec. 5, 2017, 9:57 a.m.
We don't need to define a variable if we're immediately returning it. It would also be a bit nicer to …	david	Dec. 5, 2017, 9:57 a.m.
Needs an "Args" section.	david	Dec. 5, 2017, 10:06 a.m.
Needs "Args" and "Returns" sections.	david	Dec. 5, 2017, 10:13 a.m.
Needs "Args" and "Raises" sections.	david	Dec. 5, 2017, 10:17 a.m.
Needs "Args" and "Raises" sections.	david	Dec. 5, 2017, 10:24 a.m.
It's kind of confusing how this can either be an individual status update or the list of all status updates. …	david	Dec. 5, 2017, 11:24 a.m.
Needs "Args" and "Raises" sections.	david	Dec. 5, 2017, 11:28 a.m.
Needs "Args" and "Raises" sections.	david	Dec. 5, 2017, 11:36 a.m.
In addition to specifying the details like this, perhaps we can show an example JSON blob?	david	Dec. 5, 2017, 11:57 a.m.
paren can go on the previous line.	david	Dec. 5, 2017, 11:58 a.m.
Instead of using the continuation character, let's wrap the whole thing in parens: if (not file_contents.get('reviews') and not file_contents.get('diff_comments') and …	david	Dec. 5, 2017, 11:58 a.m.
get -> hasattr	david	Dec. 5, 2017, 12:03 p.m.
get -> hasattr	david	Dec. 5, 2017, 12:03 p.m.
get -> hasattr	david	Dec. 5, 2017, 12:03 p.m.
Needs a period.	david	Dec. 5, 2017, 12:04 p.m.
Needs "Args" and "Raises" sections.	david	Dec. 5, 2017, 12:06 p.m.
Needs a period.	david	Dec. 5, 2017, 12:06 p.m.
I'm guessing you haven't yet tested this functionality because you'll need to associate the newly created review ID with the …	david	Dec. 9, 2017, 8:45 p.m.
mode is also unicode	david	Dec. 5, 2017, 12:28 p.m.
mode is also unicode, but we shouldn't need to have it as an arg at all because we'll always be …	david	Dec. 5, 2017, 12:28 p.m.
You can avoid having the entire get_file function by doing: return json.load(open(path, 'r')) We shouldn't need expandvars or expanduser because …	david	Dec. 5, 2017, 12:28 p.m.
F841 local variable 'required_fields' is assigned to but never used	reviewbot	Dec. 10, 2017, 3:05 p.m.

flake8 passed.

JSHint passed.

rbtools/commands/status_update.py (Diff revision 1)
The issue has been resolved. Show all issues
```
You might want to pull out each action into its own function.
```

Change Summary:

Move each action into their own functions.

Commit:

-	0514de804b51b2e307e0c98fb7def0e30814db67
+	757a8df6f2f4f748f3592e03f3d460afe8a2b904

Diff:

Revision 2 (+256)

Show changes

	setup.py
	rbtools/commands/status_update.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add current dev changes.

Commit:

-	757a8df6f2f4f748f3592e03f3d460afe8a2b904
+	4ac4f48cb547d6069cedc5d8886be022cdfba9f7

Diff:

Revision 3 (+291)

Show changes

	setup.py
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix valid state options.

Commit:

-	4ac4f48cb547d6069cedc5d8886be022cdfba9f7
+	8908fce6c2533ac34266d530a6dfc723ceddff67

Diff:

Revision 4 (+291)

Show changes

	setup.py
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Add basic review creation functionality.

Testing Done:

	+	Debug testing of the `--review` input on local installation with the following file contents:
	+
	+
	+
	+	`{`
	+	"review": {
	+	"body_top": "Header comment"
	+	},
	+	"diff_comment": [
	+	{
	+	"filediff_id": 10,
	+	"first_line": 729,
	+	"issue_opened": true,
	+	"num_lines": 1,
	+	"text": "Adding a comment on a diff line",
	+	"text_type": "markdown"
	+	}
	+	],
	+	"general_comments": [
	+	{
	+	"text": "Adding a general comment",
	+	"text_type": "markdown"
	+	}
	+	]
	+	}
	+

Commit:

-	8908fce6c2533ac34266d530a6dfc723ceddff67
+	bf25b22d2a0b042ec2edac72ef53615d4f756a5f

Diff:

Revision 5 (+361)

Show changes

	setup.py
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

I know most files in rbtools don't have them (yet), but we should have a file docstring at the top.

rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Add a blank line between these two.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Returns" sections.
```

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

We don't need to define a variable if we're immediately returning it. It would also be a bit nicer to wrap this as:

return {
    key: status_update.get(key)
    for key in keys
    if status_update.get(key)
}

rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs an "Args" section.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Returns" sections.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Raises" sections.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Raises" sections.
```

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

It's kind of confusing how this can either be an individual status update or the list of all status updates. I think I'd prefer if the conditional (do we have self.options.sid) is the top-level construct and then we either update or create as appropriate.

rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Raises" sections.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Raises" sections.
```

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

In addition to specifying the details like this, perhaps we can show an example JSON blob?

rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
paren can go on the previous line.
```

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

Instead of using the continuation character, let's wrap the whole thing in parens:

if (not file_contents.get('reviews') and
    not file_contents.get('diff_comments') and
    not file_contents.get('general_comments')):



You can also use hasattr instead of get, which is a bit more efficient.

rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
get -> hasattr
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
get -> hasattr
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
get -> hasattr
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a period.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs "Args" and "Raises" sections.
```
rbtools/commands/status_update.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Needs a period.
```

rbtools/commands/status_update.py (Diff revision 5)

The issue has been resolved. Show all issues

I'm guessing you haven't yet tested this functionality because you'll need to associate the newly created review ID with the status update.

It's also a good idea to do that association before publishing the review, so it doesn't show up (momentarily) as a regular review.

rbtools/utils/filesystem.py (Diff revision 5)
The issue has been dropped. Show all issues
```
mode is also unicode
```

rbtools/utils/filesystem.py (Diff revision 5)

The issue has been dropped. Show all issues

mode is also unicode, but we shouldn't need to have it as an arg at all because we'll always be reading the file. We can just pass in 'r' to open.

rbtools/utils/filesystem.py (Diff revision 5)

The issue has been resolved. Show all issues

You can avoid having the entire get_file function by doing:

return json.load(open(path, 'r'))


We shouldn't need expandvars or expanduser because the user's shell should have done the globbing for us.

```
Code looks good.
```

Change Summary:

Update review request based on feedback and clean stuff up.

Commit:

-	bf25b22d2a0b042ec2edac72ef53615d4f756a5f
+	1bf3650aa598314f893a130839e449ab61654ea9

Diff:

Revision 6 (+441)

Show changes

	setup.py
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

What's left to do here to get this out of WIP?

bleblan2 Dec. 9, 2017, 8:45 p.m.

Being able to attach the review that gets created by the json file to the status-update, I'm about to push a diff with those changes.

Change Summary:

Add attaching of created review to status-update and move out of WIP.

Summary:

-	[WIP] Add status-update command to rbtools.
+	Add status-update command to rbtools.

Commit:

-	1bf3650aa598314f893a130839e449ab61654ea9
+	5d28a093765d80ed303bd112795b1b8378fd2733

Diff:

Revision 7 (+469)

Show changes

	setup.py
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Missing docs rst file for new status-update command.

Change Summary:

Add docs rst file for status-update and add APIError catching to review creation.

Commit:

-	5d28a093765d80ed303bd112795b1b8378fd2733
+	640828e7cd8cb61e016991b922cc6674b3fdce6f

Diff:

Revision 8 (+538)

Show changes

	setup.py
	docs/rbtools/rbt/commands/status-update.rst
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/status_update.py (Diff revision 8)
The issue has been resolved. Show all issues
```
F841 local variable 'required_fields' is assigned to but never used
```

Change Summary:

Fix flake8 error.

Commit:

-	640828e7cd8cb61e016991b922cc6674b3fdce6f
+	dd36fbb04fd76905d5466172c7c7d0120be83c22

Diff:

Revision 9 (+536)

Show changes

	setup.py
	docs/rbtools/rbt/commands/status-update.rst
	rbtools/commands/status_update.py
	rbtools/utils/filesystem.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Landing with minor changes. Thanks!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-1.0.x (115b9c9)