Added a wsgi application that automatically discovers all git repositori... #112
Conversation
…ories in KLAUS_REPOS. ``klaus.contrib.autodiscover:application`` Also watches for changes to KLAUS_REPOS using the most efficient filesystem notifier available on the system (pip install watchdog)
|
Thanks for the patch. Have you tried this code? https://github.com/jonashaag/klaus/wiki/Autoreloader |
|
I have tried the existing Autoreloader. However, it doesn't do intelligent autodiscovery of git repos, and if a directory is added that is not a git repository, the behaviour is likely to be a crash, or at least a stern warning with an HTTP500 from the wsgi server. Additionally, the polling implementation, while perfectly efficient, doesn't update klaus's list of repos immediately. I think they both have valid use cases, but I believe the features introduced by his PR make administration a lot simpler. That said, if there are any improvements you wish me to make before you're willing to merge, let me know, and I''ll get right on it. |
|
OK, I've taken the general idea into consideration and I think it's a nice addition. I hope you have the patience to do a few iterations on coding style, as this patch will need quite a bit of work to meet my standards 😎 |
klaus/contrib/autodiscover.py
Outdated
| logger.debug("Dulwich says '%s' is not a git repository", directory) | ||
| return | ||
|
|
||
| matching = filter(lambda existing_repo: existing_repo.name == repo.name, self.app.repos) |
There was a problem hiding this comment.
This (and all the other places as well, of course) should use .repo_map to avoid all the cumbersome abuses of the list datastructure :-)
Please get rid of the .repos attribute in Klaus entirely. I don't know why it's there (probably for historic reasons) -- it's not needed at all. Perhaps it served as a way to keep repo order in the past, but that use case doesn't exist anymore because the repo list is ordered explicitly in the one view it's used. We can simply drop it in favor of the repo_map attribute. Please make this a separate commit as it is not really related to the autodiscovery thing.
There was a problem hiding this comment.
This makes a lot of sense. I must confess, I didn't actually fully investigate how .repos is used internally as this patch was initially just a proof of concept. However a quick grep -R -E '\.repos' shows:
__init__.py: self.repos = map(FancyRepo, repo_paths)
__init__.py: self.repo_map = dict((repo.name, repo) for repo in self.repos)
__init__.py: dict(('/'+repo.name, repo) for repo in app.repos)
views.py: repos = sorted(current_app.repos, key=sort_key, reverse=reverse)
While we're dealing with klaus.Klaus.repo_map, i'd like to propose to change the dictionary key from FancyRepo.name to FancyRepo.path as FancyRepo.name does not guarantee correct behaviour for multiple repositories with the same basename.
Here we can key klausapp.repo_map[repo.path] to check for membership, and do a simple del klausapp.repo_map[repo.path] to remove an item.
I'm quite happy to remove the .repos attribute from the remaining parts of klaus that use it in another commit.
Interested to learn what you think about keying off FancyRepo.path instead of FancyRepo.name
There was a problem hiding this comment.
That makes a lot of sense. Didn't think of that. Agreed!
There was a problem hiding this comment.
FYI, I did some more thinking about this, and tried to code around the fact the repo_map.keys() are used in the URL to uniquely identify a repository. I've left it for now. If you can think of a way to uniquely identify repositories with the same basename that doesn't leak filesystem information in the URL, and keeps the URLs as pretty as they are now, I think this could be revisited, but for now, we'll just have to live with only using repositories that have unique basenames.
There was a problem hiding this comment.
True. Still I think path is the correct key into that dictionary
klaus/__init__.py
Outdated
| self.repos = map(FancyRepo, repo_paths) | ||
| self.repo_map = dict((repo.name, repo) for repo in self.repos) | ||
| def __init__(self, repo_paths, site_name, use_smarthttp, **kwargs): | ||
| self.repo_map = dict((repo.path, repo) for repo in map(FancyRepo, repo_paths)) |
There was a problem hiding this comment.
dict((path, FancyRepo(repo)) for path in repo_paths) :-)
|
@jonashaag Just to let you know, I've not forgotten about this (not in the slightest), and will soon have time to improve this PR to the point that we agree it should be merged. Some good news is that I've been running a variation on this patch in production for a few months, with no issues, so I remain confident that this is a good long term option. Please assign me if you're willing and able. |
|
Hi @jonashaag I've made some more cleanups, and merged your master branch. I think things look quite different to my initial PR and I believe they're ready for merge, so if you get some time, I'd welcome your comments. |
| needle = os.path.realpath(needle) | ||
| if not os.path.isdir(needle): | ||
| return False | ||
| return needle.startswith(haystack) |
There was a problem hiding this comment.
You'll have to make sure both haystack and needle end with a / for the .startswith to work correctly. Counter example with the current implementation: is_subdirectory("/foo", "/foo2") is True if both paths exists, but should be False
To append the slash, os.path.join(path, '') can be used
| event.dest_path | ||
| ) | ||
| self._klausapp.add_repo(event.dest_path) | ||
| break |
|
closing in favour of #267 |
...es in KLAUS_REPOS.
klaus.contrib.autodiscover:applicationAlso watches for changes to KLAUS_REPOS using the most efficient filesystem notifier available on the system (pip install watchdog)
I made slight changes to
klaus.Klausto enable the integration with the filesystem watcher, but these changes should be backwards-compatible.Usage of the new autodiscover module requires
watchdogThis new wsgi application significantly eases administrative burden when creating/deleting/moving git repositories. The web server no-longer requires a restart.