Resync with public #62
Conversation
Changes pulled in from the public Sideboard repo Replaces https://gitlab.services.zz/psd/sideboard/merge_requests/26 with exactly the same changes, but with a ``git rebase`` thrown in to clean up our commit history. See merge request !27
@declarative_base now fills in a __init__ which applies default column values
One annoying thing about SQLAlchemy's default ``__init__`` is that it doesn't apply default values until immediately before we save. For example:
```
class Foo(Base):
id = Column(UUID(), default=uuid4)
assert Foo() is None
```
This is unfortunate, because we'd often like to be able to do things like this:
```
with Session() as session:
bar = Bar()
...
session.add(bar)
return bar.id
```
But that doesn't work, because we don't get the id until after the commit, so we need to manually commit in a bunch of places just so we can get the default values.
MAGFest implemented a feature wherein the default ``__init__`` automatically applies defaults for fields which we didn't explicitly set at initialization:
https://github.com/magfest/ubersystem/blob/64b7614e4c124183dfeccefb942261ad87f88f19/uber/models.py#L141
This implements that function in a more generic way and adds a couple of unit tests for it. I also added some docstrings, since we're trying to gradually fill in docstrings for everything which doesn't already have them.
See merge request !28
Pluggable authentication I made three changes here: 1) I implemented the idea of pluggable authentication as ticketed at #31 The idea is that when writing web applications we often want to be able to mark pages as requiring you to be logged in, and this seems like a generally useful thing that we should bundle into Sideboard. In particular, plugins now have a way to say, "Here's how you check whether someone is logged in and here's the login page you redirect them to when they visit a restricted page." 2) Since this involves HTML login pages, I made a change that I'd been thinking about making for awhile to how we do Jinja templates. Awhile back we decided to change the default Jinja tokens from the things like ``{{`` and ``}}`` so as to not conflict with Angular. However, we've now settled on a more standard way to ship Angular code and we don't mix rendered Jinja in with our Angular templates, so this feature is unnecessary. None of our production Sideboard plugins are currently using Jinja templates, so this change seems acceptable. 3) I added ``pep8`` linting and made the codebase pep8-compliant. I'd planned on doing this the next time I did any major work on Sideboard, but I didn't realize it would produce quite so many changes. I apologize for not doing this as part of a separate MR, since this is going to make this review harder to scan through - if I had it to do over again I would NOT have mixed this in with the other features above. (I plan on updating our Jenkins job for Sideboard to run ``pep8`` alongside the unit tests like we do for our plugins.) In order to make this easier to review I'll call out the non-pep8 changes that are actually relevant. See merge request !30
added ability for plugins to define a preprocessing step for API calls
We find ourselves with the following example use case. Suppose we have an eCard service with methods like this
```
ecard.show_all(username)
ecard.send(username, recipient, card)
```
We also have a website where people can log in. In the Javascript code on the website we want to be able to make API calls like
```
WebSocketService.subscribe('ecard.show_all')
WebSocketService.call('ecard.send', recipient, card)
```
The website backend can then prepend the currently logged in user to the API calls, since it would be silly (and insecure) to have the Javascript client be responsible for that. This change lets us accomplish this by setting a ``preprocess`` callback on the websocket object like this:
```
def prepend_username(method, params):
if method.startswith('ecard.'):
params['username'] = threadlocal.get('username')
return params
from sideboard.lib import services
services.get_websocket('ecard').preprocess = prepend_username
```
We've already used this feature for one of our own services and it's working well.
See merge request !31
Added preset value to UID and GID modified: preinstall.sh Author: elic <eli@courtwright.org> modified: .gitignore modified: package-support/preinstall.sh modified: sideboard/lib/_websockets.py modified: sideboard/tests/test_websocket.py
Added preset value to UID GID modified: preinstall.sh See merge request !34
… and when the SIDEBOARD_MODULE_TESTING setting should be set
root and module_root now tied together Before Sideboard was released, we had a use case with different virtualenvs in the same process. We scrapped this before version 1.0 but there's still some legacy code in Sideboard which was written to deal with that. This cleans some of that up, since it was interfering with our latest packaging work. This is necessary because on our latest build server, the ``/etc/sideboard`` directory exists, which means that the wrong ``root`` directory is being returned, which means that Sideboard isn't finding the ``development-defaults.ini`` file that it needs. By standardizing on "the root is always one level up from the module root", this eliminates the problem and allows us to have Sideboard actually installed on the build server without messing up our ability to run in development mode. See merge request !35
Decrease sideboard.lib.WebSocket.call latency
There's currently some code in our WebSocket client class which waits for a response like this:
```
for i in range(10 * config['ws.call_timeout']):
stopped.wait(0.1)
# check for response
```
I originally picked 0.1 seconds as an offhand "this seems really responsive" value, but in retrospect it's probably around 100 times too long! On a fast local network I'd expect to have response times in the 1s of milliseconds (this is born out by recent experiments).
This change uses a ``threading.Event`` object so that the other thread can signal us to stop waiting as soon as an event arrives. This means that we don't need to decrease the wait time, since we'll be notified immediately.
Our existing unit tests cover all the code paths. As a performance test I spun up 100 threads each making a call. The CPU on my dev box remained at <2% so I don't have any performance concerns with this approach.
See merge request !36
fixed unit tests for WebSocket.call Whoops! I did a refactor on ``WebSocket.call`` in my previous pull request, then did ANOTHER refactor to make it more efficient and forgot to rerun the unit tests after the second refactor. I've re-run them now and this fixes the breakage. See merge request !37
…n separate locations
Update get_dirnames() so that root and module_root can correctly be in separate locations After some more back and forth to get things working as part of our new packaging effort, we decided that we did want ``root`` and ``module_root`` to be able to be different after all. We'd like to support the following deployment scenario: - Plugins are installed to ``/usr/lib/pythonX.Y/site-packages`` - Plugin directory is still somewhere like ``/opt/sideboard/plugins`` - Each plugin symlinks into the site-packages directory, e.g. for a ``foo`` plugin, ``/opt/sideboard/plugins/foo/foo`` would be a symlink to ``/usr/lib/pythonX.Y/site-packages/foo`` - In the above example, ``root`` would be ``/opt/sideboard/plugins/foo`` and ``module_root`` would be ``/usr/lib/pythonX.Y/site-packages/foo``. I've expanded the docstring to more fully explain this as well as adding an explanation for "what are 'root' and 'module_root' anyway?" See merge request !38
…iption rather than create a new one
fixed forwarded websocket subscriptions to update the existing subscription rather than create a new one See merge request !46
| <html> | ||
| <head><title>$(( plugin ))$ skeleton page</title></head> | ||
| <head><title>{{ plugin }} skeleton page</title></head> |
There was a problem hiding this comment.
One change we made awhile back was to swap out our overridden Jinja2 default tags with the actual defaults that Jinja2 ships. However, at the time we neglected to update our paver skeleton template - here's that change.
I'm not sure that anyone actually uses the paver create_plugin task, but I do try to keep it up to date since it's what the Sideboard tutorial has people do.
There was a problem hiding this comment.
I am of the opinion that the default tags should be changed for Angular, rather than changing the Jinja2 default tags. Using default Jinja2 tags and custom Angular tags is a more flexible design.
We could potentially make Angular tags a configurable option, allowing developers to specify their own open/close tags. It would be much harder to make the Jinja2 tags configurable, as the templates are rendered on the server. So we'd have to render them once to insert the correct open/close tags, then have Jinja2 render them to insert proper values.
There was a problem hiding this comment.
One general note about mixing Angular and Jinja: in general I've come to believe it's a really bad code smell if you're rendering dynamic data server side into an Angular template. There are times when that's useful but it costs you a lot more in the long run, e.g. it becomes way harder to write meaningful unit tests for such code. That's part of why we change this; not only do we prefer to not change our default Jinja tags (as Rob says), we don't think it should be necessary in the first place.
| groupadd --force -r sideboard | ||
| useradd -r --shell /sbin/nologin --gid sideboard sideboard | ||
| groupadd --force -r sideboard -g 600 | ||
| useradd -r --shell /sbin/nologin -uid 600 --gid sideboard sideboard |
There was a problem hiding this comment.
The devops guys at my work say that it's useful if the sideboard user and group have consistent ids across all of our systems. That sounded reasonable, so we hard-coded this number. I also doubt that anyone will use this preinstall script anyway (MAGFest deploys out of Github checkouts).
|
|
||
| [formatters] | ||
| [[syslog]] | ||
| format = "$$(levelname)-5.5s $$(threadName)s [$$(name)s] $$(message)s" | ||
|
|
There was a problem hiding this comment.
Here we add a syslog formatter to our default config - note that it's not actually used by default so this is a backwards-compatible change.
The main motivation here is that syslog already prepends timestamps to log messages, so we want a different formatter so that we're not wasting log output. (Those of you familiar with the Splunk Cost Fallacy know that Splunk is really expensive so that kind of thing actually adds up to real dollars.)
pavement.py
Outdated
| sh('cd {path} && {python_path} {setup_path} develop' | ||
| .format( | ||
| path=path, | ||
| python_path=sys.executable, | ||
| python_path=venv_python if exists(venv_python) else sys.executable, |
There was a problem hiding this comment.
Sideboard now support running either out of a virtualenv or out of the system Python; this change supports that.
| except: | ||
| from sideboard.lib import log | ||
| log.warning('Error importing server', exc_info=True) | ||
| import sideboard.server |
There was a problem hiding this comment.
This swallowing of exceptions at import time dates back to when Sideboard was still Dradis and we wanted to run in a non-server mode. We handle that differently now, and this exception swallowing was just making it harder to debug Sideboard itself since I'd occasionally make a typo in server.py and then not notice right away.
| @@ -13,57 +13,69 @@ class ConfigurationError(RuntimeError): | |||
| pass | |||
|
|
|||
|
|
|||
| def os_path_split_asunder(path, debug=False): | |||
| def get_dirnames(pyname, plugin): | |||
There was a problem hiding this comment.
I believe I added enough docstring comments in this file to explain what's going on, but let me know if anything is confusing.
| @@ -1,13 +1,28 @@ | |||
|
|
|||
| debug = boolean(default=False) | |||
|
|
|||
| # Directory where core Sideboard pages look for plugins. | |||
There was a problem hiding this comment.
Over time I'm trying to fill in code comments in places where they're missing. I've gotten a lot better about filling them in for all new code as I go, but there's still a backlog of uncommented stuff back from my younger foolish days when I found comments too ugly to bear ruining my code with :P
| @@ -48,12 +48,12 @@ def _check(url, **ssl_params): | |||
|
|
|||
| try: | |||
| with closing(socket.create_connection((host, port))) as sock: | |||
| wrapped = ssl.wrap_socket(sock, cert_reqs=ssl.CERT_REQUIRED, **ssl_params) | |||
| wrapped = ssl.wrap_socket(sock, **dict(ssl_params, cert_reqs=ssl.CERT_REQUIRED)) | |||
| status.append('succeeded at validating server cert') | |||
There was a problem hiding this comment.
MAGFest doesn't make much use of our server API, but this adds an extra check to our automatic SSL checking.
sideboard/lib/__init__.py
Outdated
| 'DaemonTask', 'Caller', 'GenericCaller', 'TimeDelayQueue', | ||
| 'WebSocket', 'Model', 'Subscription', 'MultiSubscription', | ||
| 'listify', 'serializer', 'cached_property', 'request_cached_property', 'is_listy', 'entry_point', | ||
| 'listify', 'serializer', 'cached_property', 'request_cached_property', 'is_listy', 'entry_point', 'RWGuard', |
There was a problem hiding this comment.
Oh huh, I think this is the merge problem. We already merged the RWGuard into master I believe; it started as a branch off a branch and then got merged into the branch before being merged into master, so I think that's why we're having merge problems. None of the RWGuard stuff is actually new and you can see the original review at #46
| @@ -10,8 +10,9 @@ | |||
| import cherrypy | |||
|
|
|||
| import sideboard.lib | |||
| from sideboard.lib import log, config | |||
| from sideboard.lib import log, config, serializer | |||
There was a problem hiding this comment.
The changes in this file all relate to #31 which MAGFest probably doesn't care much about, but was useful for my day job.
| @@ -4,7 +4,7 @@ | |||
| import json | |||
| from copy import deepcopy | |||
| from itertools import count | |||
| from threading import RLock | |||
| from threading import RLock, Event | |||
There was a problem hiding this comment.
The changes in this file all related to "passthrough subscriptions" which MAGFest definitely doesn't care about at the moment. If you're interested, you can read the docstring for the preprocess and make_caller methods below.
| @@ -18,7 +18,7 @@ | |||
| 'comparison': <comparison_function> | |||
| 'field': <model_field_name>, | |||
| 'value': <model_field_value> | |||
| }]+ | |||
| }] | |||
There was a problem hiding this comment.
Literally the only changes in this file are pep8 style fixes.
| @@ -3,61 +3,33 @@ | |||
| import sys | |||
|
|
|||
| import six | |||
| #import ldap | |||
There was a problem hiding this comment.
We've had a bunch of commented-out and dead code that's literally never been used in the history of Sideboard but which was sitting around because it got copied in from an older project. I finally deleted it. Sideboard plugins are welcome to use LDAP (one of them where I work totally does) but I don't think it makes sense to bundle it into the project, and we now support custom authenticators so it's super-easy to do this in a plugin.
| @@ -24,6 +24,30 @@ | |||
|
|
|||
|
|
|||
| class threadlocal(object): | |||
| """ | |||
There was a problem hiding this comment.
MAGFest doesn't currently do anything with websockets so it doesn't care about anything in this file. For the sake of completeness the three main changes in this file are:
- docstrings for all the utility methods that didn't have them
- support for passthrough subscriptions
- support for configurable/pluggable authentication
|
btw that pinning cherrypy should actually be removed, if possible, because it was working around a bug in ws4py that should now be fixed (saw them merge the fix in), detailed in #56 . though, there may be other reasons besides #56 to keep cherrypy pinned at version 8, so feel free to do so if it's necessary. |
|
Good to know about the cherrypy versions! To keep things simple, I'm going to keep the version pinned to 8.9.1 for this pull request (tests aren't passing otherwise). We'll fix the cherrypy version when we close out #56. |
|
I'm ready to merge this, but I'd like some guidance on our merge policies:
|
|
up to this point we've mostly been using merge commits for everything (mostly because the ability to do otherwise was just recently added to github). in general, we've not been too concerned about keeping the history too clean and are cool with WIP commits. I'd say it's up to you going forward to set the policy, and would agree that what you outlined as recommendations seem solid, and for this PR, a merge commit seems most appropriate |
|
Alright! I'll add the merge policy to our contribution guidelines (when those are eventually created) |
|
breezed through the diff quickly, +1 from my perspective |
Another reason to use merge commits: in a case like this where code is being pushed back from a private repo to this public repo, if you edited the commit history then the next time we tried to pull down the public repo and merge it with our private repo we'd get a giant merge conflict because your history and ours would be different. I think anything that increases the friction in having people exchange code is probably bad, hence the default merge commit strategy being best. With that being said, it's possible that it would be better for us to just have a cleaner commit history in general, e.g. we should be collapsing commits for every feature we add instead of having a bunch of random "pep8 fixes" commits and whatnot. |
We've done a bit of work on Sideboard at my day job over the past several months since the last time we resynced with the public repo (see #27), so I wanted to push up the changes so that we don't get too far out of sync.
I see there's a "can't automatically merge" which might mean that I did something dumb when I merged our repos together. I vaguely recall doing some git rebasing to collapse history but I thought I'd done that properly before I pushed up the changes such that there shouldn't be any problems. Hopefully we won't have to do too many shenanigans to get this merged together.
The main change in terms of number of lines is that we added
pep8linting to the codebase with basically the same rules that MAGFest uses. We also fixed a few typos and such. I'll go through and add explanations for anything that's not already commented that might not be obvious.