Summary

Install an extension on-the-fly from a remote source.

Review Request #3973 — Created March 17, 2013 and submitted April 14, 2013, 3:30 a.m.

Information

Owner

ayrus

Repository

Djblets

Branch

master

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

Added a method to djblet's extension manager that installs an extension, given a remote URL with the extension egg and package name.

This modification is required as a pre-requisite, for the upcoming extension browser module to ReviewBoard.

There could be possible errors such as (but not limited to): The URL being a 404, incorrect package_name, incompatible egg or the package being already present. These would be thrown as exceptions and have to be handled by the caller appropriately.

Testing Done

The changes have been tested with the different variations of extension browser module so far.

Presently, with the module being absent it can tested manually by invoking the install_extension() method on the extension manager with the required parameters. If the extension isn't already installed, the installation would go through.

Issues

Description	From	Last Updated
No space after """	chipx86	March 24, 2013, 6:17 p.m.
"containing"	chipx86	March 24, 2013, 6:18 p.m.
We should ideally wrap the errors in some sane type they can catch, or otherwise, list every error they need …	chipx86	March 27, 2013, 2:15 a.m.
This looks like it can fit on one line.	chipx86	March 24, 2013, 6:18 p.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:23 a.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:23 a.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	March 27, 2013, 2:24 a.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:23 a.m.
"pkg_resources" Can you make these bullet point lists?	chipx86	March 27, 2013, 3:12 a.m.
What does this mean? We should never be at a point where there's an uncaught exception causing an HTTP 500.	chipx86	March 27, 2013, 3:12 a.m.
Can you make this align with the others? (Ignore pep8 here.)	chipx86	March 27, 2013, 3:15 a.m.
Col: 49 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:25 a.m.
Alignment issue.	chipx86	March 27, 2013, 3:16 a.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:25 a.m.
Col: 41 E127 continuation line over-indented for visual indent	reviewbot	March 27, 2013, 2:25 a.m.
Second sentence is a little awkward. How about: "Installation may fail if a malformed install_url or package_name is passed, which …	mike_conley	April 3, 2013, 6:32 p.m.
Col: 22 E222 multiple spaces after operator	reviewbot	March 27, 2013, 3:22 a.m.
For consistency's sake, this = should be aligned with all of the others.	mike_conley	April 3, 2013, 6:34 p.m.
Col: 27 E222 multiple spaces after operator	reviewbot	March 27, 2013, 3:22 a.m.
Col: 20 E221 multiple spaces before operator	reviewbot	April 3, 2013, 6:35 p.m.
Col: 25 E221 multiple spaces before operator	reviewbot	April 3, 2013, 6:35 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/extensions/base.py
  Ignored Files:

Any luck figuring out the .pth file override issue?

djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No space after """
```
djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"containing"
```

djblets/extensions/base.py (Diff revision 1)

The issue has been resolved. Show all issues

We should ideally wrap the errors in some sane type they can catch, or otherwise, list every error they need to worry about. Right now, it requires digging into the source (and I actually have no idea what those errors are).

djblets/extensions/base.py (Diff revision 1)
The issue has been resolved. Show all issues
```
This looks like it can fit on one line.
```

Diff:

Revision 2 (+31 -7)

Show changes

	djblets/extensions/base.py
	djblets/webapi/errors.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/extensions/base.py
    djblets/webapi/errors.py
  Ignored Files:

djblets/webapi/errors.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/webapi/errors.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
djblets/webapi/errors.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```
djblets/webapi/errors.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```

Diff:

Revision 3 (+31 -7)

Show changes

	djblets/extensions/base.py
	djblets/webapi/errors.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/extensions/base.py
    djblets/webapi/errors.py
  Ignored Files:

djblets/webapi/errors.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 49
 E127 continuation line over-indented for visual indent
```
djblets/webapi/errors.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```
djblets/webapi/errors.py (Diff revision 3)
The issue has been dropped. Show all issues
```
Col: 41
 E127 continuation line over-indented for visual indent
```

djblets/extensions/base.py (Diff revision 3)
The issue has been dropped. Show all issues
```
"pkg_resources"

Can you make these bullet point lists?
```

djblets/extensions/base.py (Diff revision 3)

The issue has been resolved. Show all issues

What does this mean? We should never be at a point where there's an uncaught exception causing an HTTP 500.

djblets/webapi/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can you make this align with the others? (Ignore pep8 here.)
```
djblets/webapi/errors.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Alignment issue.
```

Diff:

Revision 4 (+33 -7)

Show changes

	djblets/extensions/base.py
	djblets/webapi/errors.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/extensions/base.py
    djblets/webapi/errors.py
  Ignored Files:

djblets/webapi/errors.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 22
 E222 multiple spaces after operator
```
djblets/webapi/errors.py (Diff revision 4)
The issue has been dropped. Show all issues
```
Col: 27
 E222 multiple spaces after operator
```

Hey Surya,

I'm pretty happy with this (although I haven't been down in the guts of setuptools like you and ChipX86 in a while, so take my opinion with a grain of salt). Just two little style / doc nits, but otherwise, I think this looks really reasonable.

-Mike

djblets/extensions/base.py (Diff revision 4)

The issue has been resolved. Show all issues

Second sentence is a little awkward. How about:

"Installation may fail if a malformed install_url or package_name is passed, which will cause an InstallExtensionError exception to be raised."

djblets/webapi/errors.py (Diff revision 4)
The issue has been resolved. Show all issues
```
For consistency's sake, this = should be aligned with all of the others.
```

Diff:

Revision 5 (+33 -7)

Show changes

	djblets/extensions/base.py
	djblets/webapi/errors.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    djblets/extensions/base.py
    djblets/webapi/errors.py
  Ignored Files:

djblets/webapi/errors.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 20
 E221 multiple spaces before operator
```
djblets/webapi/errors.py (Diff revision 5)
The issue has been dropped. Show all issues
```
Col: 25
 E221 multiple spaces before operator
```

Ship it!

```
Looks good to me! Ship it!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to master (fc5f39f)