Summary

Fixed a bug where rbt land was unable to determine the approval state of a review request.

Review Request #8401 — Created Sept. 17, 2016 and submitted Sept. 27, 2016, 12:13 p.m.

Information

Owner

rswanson

Repository

RBTools

Branch

release-0.7.x

Bugs

4024

Depends On

Commit

49c4e26...

Reviewers

Groups

rbtools, students

People

Description

Fixed a bug where rbt land was unable to determine the approval state
of a review request.

This change introduces default values to is_rr_approved and
approval_failure before attempting to retrieve information from the
review request. In the event an exception is encountered, RBTools will
fail gracefully without disclosing a stack trace.

Testing Done

Ran the land command multiple times, and observed its functionality
with and without ship-its. Noted that functionality is unaffected
for normal use cases.

Issues

Description	From	Last Updated
Can you wrap your description and testing done at 72 characters?	brennie	Sept. 20, 2016, 7:54 p.m.
You cleaned up the description but not the summary. Can you do that as well?	brennie	Sept. 20, 2016, 7:54 p.m.
Nit: Needs period.	mike_conley	Sept. 18, 2016, 12:43 p.m.
Add a blank line after this.	david	Sept. 19, 2016, 9:10 p.m.
So there are a couple states that can be hit where defaults come into play, not all of which are …	chipx86	Sept. 21, 2016, 10:21 p.m.
Now that we have a generic exception handler I don't think we need to set the default values.	david	Sept. 23, 2016, 9:07 a.m.
These are indented too far.	david	Sept. 23, 2016, 9:07 a.m.
This should be raise CommandError(...)	brennie	Sept. 26, 2016, 8:26 p.m.
Alphabetize these.	brennie	Sept. 27, 2016, 10:53 a.m.
e on next line.	brennie	Sept. 27, 2016, 10:53 a.m.

This makes sense to me - only one small nit.

rbtools/commands/land.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Nit: Needs period.
```

Change Summary:

Addressed mconley's nit.

Commit:

5c43cbcd5ee02ba7af2a584d41ab825234f3f178

6f033809de52b5921b0a5f4befa6d3b9bf0e8307

Diff:

Revision 2 (+5)

Show changes

rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py

Summary:: [Draft] Setting a default 'is_rr_approved' value for land command outside try block.
Setting a default 'is_rr_approved' value for land command outside try block.

Change Summary:: Updated bug tracker on review request.
Bugs:: 4024

Can you fix up your summary to be a bit less specific about the code change? Something like "Fix a bug where rbt land was unable to find the approval state".

In the description, it looks like your code blocks (things in `) have escape characters. Mind editing that so that any reference to is_rr_approved shows up with the nice code block syntax? Things like Exception and AttributeError should be similarly marked.

rswanson Sept. 19, 2016, 9:07 p.m.

Sounds good. I've updated the description / summary accordingly.

rbtools/commands/land.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Add a blank line after this.
```

Description:

~		Currently, 'is_rr_approved' may not be set during execution of RBTools' land command
	~	Fixed a bug where rbt land was unable to determine the approval state of a review request.
-		if an Exception other than AttributeError is encountered.

~		This change introduces default values to `is_rr_approved` and `approval_failure` before
	~	This change introduces default values to `is_rr_approved` and `approval_failure` before
		attempting to retrieve information from the review request. In the event an exception is
		encountered, RBTools will fail gracefully without disclosing a stack trace.

Commit:

6f033809de52b5921b0a5f4befa6d3b9bf0e8307

1dc2a8a05ce9b251889213ab3039a4d28266f4e5

Diff:

Revision 3 (+6)

Show changes

rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py

The issue has been resolved. Show all issues

Can you wrap your description and testing done at 72 characters?

The issue has been resolved. Show all issues

You cleaned up the description but not the summary. Can you do that as well?

Summary:: Setting a default 'is_rr_approved' value for land command outside try block.
Fixed a bug where rbt land was unable to determine the approval state of a review request.
Description:: ~
Fixed a bug where rbt land was unable to determine the approval state of a review request.
~
Fixed a bug where rbt land was unable to determine the approval state
+ of a review request.

~
This change introduces default values to is_rr_approved and approval_failure before
~ attempting to retrieve information from the review request. In the event an exception is
~ encountered, RBTools will fail gracefully without disclosing a stack trace.
~
This change introduces default values to is_rr_approved and
~ approval_failure before attempting to retrieve information from the
~ review request. In the event an exception is encountered, RBTools will
+ fail gracefully without disclosing a stack trace.
Testing Done:: ~
Ran the land command multiple times, and observed its functionality with and without ship-its. Noted that functionality is unaffected for normal use cases.
~
Ran the land command multiple times, and observed its functionality
+ with and without ship-its. Noted that functionality is unaffected
+ for normal use cases.

Can you wrap your Description/Testing Done to about 75 characters per line? The text is used directly for commit messages, and having those at the proper length will help us land your changes sooner. Thanks!

chipx86 Sept. 21, 2016, 12:14 p.m.

Sorry, I had this in a draft and failed to publish before when I made the below comment. I know you've already addressed this. Thanks!

RS

rswanson Sept. 21, 2016, 9:58 p.m.
```
Cheers!
```

rbtools/commands/land.py (Diff revision 3)

The issue has been resolved. Show all issues

So there are a couple states that can be hit where defaults come into play, not all of which are errors.

1) It's an older Review Board, which doesn't have these fields (thus the AttributeError where we simulate it).
2) We get an unknown exception (maybe review_request isn't what we think, or something else goes very wrong).
3) This doesn't get hit in practice (but a linter would have a problem with it): is_rr_approved can be True while approval_failure can be undefined. This would happen if we hit the AttributeError and ship_it_count > 0. While not used directly, it would be nice to have some default here.

The default added for approval_failure is only appropriate in case #2, where there's some unknown exception. Now, it's not clear from the bug report what sort of exception was hit, but I think what we should have here is an except Exception as e: in that whole try/except block.

This handler wouldn't need to set a default, but rather would:

1) Log the exception (using logging.exception and saying something like 'Unexpected error when looking up review request approval state: %s' % e)
2) Immediately raise a CommandError with the text you're currently setting as the default for approval_failure.

It might still make sense to set the defaults above, but I'd set approval_failure to None. This would just be to satisfy any linters, but it's too early to assume anything about the failure at this part of the code.

rswanson Sept. 21, 2016, 9:58 p.m.

Thanks for taking the time to write this all out.

I see what you're saying about the possible states that can be hit here. I've updated my review request to incorporate the additional exception handling as suggested. I've left the original default approval_failure assignment in for now but changed it to None.

Commit:

1dc2a8a05ce9b251889213ab3039a4d28266f4e5

6aa61bc09c1f0dcfbe934b26f990effde7f39d70

Diff:

Revision 4 (+13)

Show changes

rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py

rbtools/commands/land.py (Diff revision 4)

The issue has been resolved. Show all issues

Now that we have a generic exception handler I don't think we need to set the default values.

rbtools/commands/land.py (Diff revision 4)
The issue has been resolved. Show all issues
```
These are indented too far.
```

Commit:

6aa61bc09c1f0dcfbe934b26f990effde7f39d70

224f4264c58efed36940c9eeda4c7f64705db751

Diff:

Revision 5 (+8)

Show changes

rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py

Ship it!

```
Ship It!
```

rbtools/commands/land.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should be raise CommandError(...)
```

Commit:

224f4264c58efed36940c9eeda4c7f64705db751

fbabfe7289804f72309e93826ed9365dea43023b

Diff:

Revision 6 (+8)

Show changes

rbtools/commands/land.py

Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py



Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py

Ship it!

rbtools/commands/land.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alphabetize these.
```
rbtools/commands/land.py (Diff revision 6)
The issue has been resolved. Show all issues
```
e on next line.
```

Commit:

fbabfe7289804f72309e93826ed9365dea43023b

49c4e269721d03b4f00f7896a288c15d3fd5c621

Diff:

Revision 7 (+9)

Show changes

rbtools/commands/land.py

Tool: Pyflakes
Processed Files:
    rbtools/commands/land.py



Tool: PEP8 Style Checker
Processed Files:
    rbtools/commands/land.py

Status:: Completed
Change Summary:: Pushed to release-0.7.x (51a7f7d)

~		Fixed a bug where rbt land was unable to determine the approval state of a review request.
	~	Fixed a bug where rbt land was unable to determine the approval state
	+	of a review request.

~		This change introduces default values to `is_rr_approved` and `approval_failure` before
~		attempting to retrieve information from the review request. In the event an exception is
~		encountered, RBTools will fail gracefully without disclosing a stack trace.
	~	This change introduces default values to `is_rr_approved` and
	~	`approval_failure` before attempting to retrieve information from the
	~	review request. In the event an exception is encountered, RBTools will
	+	fail gracefully without disclosing a stack trace.

~		Ran the land command multiple times, and observed its functionality with and without ship-its. Noted that functionality is unaffected for normal use cases.
	~	Ran the land command multiple times, and observed its functionality
	+	with and without ship-its. Noted that functionality is unaffected
	+	for normal use cases.