rearranging code into a proper python package, per #1

[mhrivnak]

No description provided.

[mhrivnak]

I'm making some assumptions here about your release process. I'm happy to undo this or change it to a different version if that's preferable.

[mhrivnak]

Looks like the travis build failed because it couldn't connect to github. Can someone with access restart that build? There should be a button on the build page: http://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit

UPDATE: looks like someone did restart it

[samalba]

What about adding a bin/script to start the registry like this:

$ pip install docker-registry
$ docker-registry -c config/config.yml

This would take care of spawning the gunicorn. So we can also remove the run.sh script. What do you think?

[wking]

On Sun, Mar 23, 2014 at 01:41:54PM -0700, Michael Hrivnak wrote:

• rearranging code into a proper python package, per Create a Python package to avoid overriding sys.lib in the code #1

I'd prefer relative imports here. For example, in
docker_registry/init.py:

from .lib import config

to the current (as of 555dbb2):

from docker_registry.lib import config

Importing just the final module (without the associated namespacing)
will also allow for shorter lines. For example, in
test/test_images.py,

images.cfg._config['nginx_x_accel_redirect'] = accel_prefix

instead of the current:

docker_registry.images.cfg._config['nginx_x_accel_redirect']
= accel_prefix

You can also avoid some wrapped lines in your setup.py by storing
os.path.dirname(os.path.abspath(file)) in a local variable:

_this_dir = os.path.dirname(os.path.abspath(file))
desc_path = os.path.join(_this_dir, 'README.md')
…
req_path = os.path.join(_this_dir, 'requirements.txt')

which reads more easily to me.

I think that this would be a good time to move registry.app.VERSION to
the more conventional docker_registry.version, and also shift the
config import to a less central location (wsgi.py?), although those
can certainly happen in subsequent commits in this PR.

[mhrivnak]

I made the changes requested by @wking and am working on replacing the run.sh with a python console-script. The latter is a little more complex than one might expect, but I have something working now that just needs a little polish.

[mhrivnak]

I replaced run.sh with a console script, so this is ready for another review.

[samalba]

cc @shin- Could you review?

[shin-]

Sorry that took so long. Works for me, only had to update the boto dependency.

LGTM.

[samalba]

Working on merging this and we do want to merge it, believe me :-)

I am testing locally and I have few more comments.

1. What's the motivation behind having a directory named "docker_registry", any way to keep all files under "registry" and still have the main package called "docker-registry"? Maybe I am totally wrong there, I don't know setuptools enough but I thought it's possible to map a package_name to a directory...
2. for the bin/script, I am thinking of a script located in "bin/docker-registry" that spawns gunicorn (it can be a shell script that forward its arguments). Then if you want to spawn gunicorn yourself, you use gunicorn docker_registry:wsgi?

Not sure if your console script addresses the second point, I tried to test it but got conflict in the checkout... Probably need a rebase.

[mhrivnak]

1. Whatever the directory holding the code is named will be what people import, and it will be on the python path (as our root module), sharing the global python namespace. As this branch currently stands, you can import docker_registry. "registry" would be a very generic name to be in the global namespace. It's a best practice for the python package name to be roughly equivalent to the name used by the root module, and I don't think "registry" would make a good package name. Perhaps the nail in that coffin is that there is already a package called "registry" on pypi, and it's a Windows registry API.

   As for "-" vs. "_", that's a quirk of python package naming. "-" is illegal in a variable name, so we can't use that in the root module name. But PEP8 says of package names: "the use of underscores is discouraged." The generally accepted way to reconcile this discrepancy is to name a package "a-b" and name its root module "a_b". For example, if you go on pypi and search for "django", you'll see a long list of packages named "django-x" whose module is named "django_x".

   It might be more clear to put this is usability terms; you pip install <package-name>, and import <root_module_name>.

   So my strong advice from a pure python packaging perspective is to go with the names that are in this branch, but I will quite happily change the branch to use any variation you prefer. Or if it would be helpful to continue this discussion, I'm happy to answer any additional questions, or elaborate on the above.

2. The docker-registry console script I created does exactly what run.sh did. The only change is that the new console script adds help/usage text. As for its location, (I apologize if this is old news to you) a benefit of having a standard python package is that you can write a function to be your console script without actually having to make something like bin/docker-registry. When you install the python package, such as with pip, python will auto-generate a skeleton file in the "right place" for your OS that calls the specified function. So in my branch, you won't find a docker-registry executable, but you will find an entry point in setup.py defining where the function is that gets called by the generated executable.

   Given that I ported the run.sh script to fit in with the python package, do you think it would be reasonable to save behavioral changes for another PR?

[wking]

On Tue, Apr 15, 2014 at 10:01:25PM -0700, Michael Hrivnak wrote:

1. … The generally accepted way to reconcile this discrepancy is to
   name a package "a-b" and name its root module "a_b"… So my strong
   advice from a pure python packaging perspective is to go with the
   names that are in this branch…

Outsider vote for package name 'docker-registry' and root module
'docker_registry'. It looks like the current tip of this PR (bf3360a)
is using 'docker_registry' for both.

1. The docker-registry console script I created…

I'd prefer if docker_registry.run_gunicorn was moved to
docker_registry.gunicorn.run. This would avoid cluttering
docker_registry/init.py, which is complicated enough with it's
existing import-to-trigger-side-effects behaviour. As a side benefit,
DESCRIPTION could be moved to docker_registry.gunicorn.doc.

Another minor quibble is the lack of a summary line 1 for
run_gunicorn.doc.

[mhrivnak]

Thanks for catching the package name! I have fixed that, and I improved the doc block for "run_gunicorn".

I don't feel strongly about where the run_gunicorn function lives.

[shin-]

I'm fine with run_gunicorn living there for now. We can open a new PR if it proves to be detrimental at some point.

[shin-]

Thanks!

[wking]

On Tue, Apr 22, 2014 at 10:34:26AM -0700, Joffrey F wrote:

Merged #297.

This maybe should have been rebased after #247 landed. Do we want a
new PR to shift lib/index?

[shin-]

@wking I've rebased it before merging, see 9baadda . If you spot any inconsistency or see something I missed don't hesitate to let me know / call me out on it, but I think we're good.

[wking]

On Tue, Apr 22, 2014 at 10:45:28AM -0700, Joffrey F wrote:

I've rebased it before merging, see
9baadda
. If you spot any inconsistency or see something I missed don't
hesitate to let me know / call me out on it, but I think we're good.

Ah, I'd missed that. 9baadda looks good to me.