Giter VIP home page Giter VIP logo

sideboard's Introduction

Sideboard Build Status Coverage Status

Getting Started

Please read the checked in docs for the moment.

sideboard's People

Contributors

binary1230 avatar bitbyt3r avatar eliandrewc avatar gillianwebb avatar kitsuta avatar robdennis avatar robruana avatar thaeli avatar

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

sideboard's Issues

check_connections fails on Python 3

The following line works in Python 2 but not Python 3 (from connection_checker.py):

jproxy = jservice._send.im_self  # ugly kludge to get the ServerProxy object

Apparently there's some Python 3 equivalent to this which we'll need to try.

Sideboard should complain at startup if there isn't a valid/complete logging setup

If there are no log handlers defined, Sideboard will silently just not log anything. This should be a warning at startup if the logging config appears to not result in things being logged. Perhaps even add a printout on startup of what the logging config it's read in is.

I'm not convinced this should be a fatal error, but it should be noted on startup to stdout/stderr at least.

Unit test for sorted dict encoding

Moved here from the original (now private) issue tracker.

Our SortedDictJsonEncoder was broken for a long time, and our tests happened to pass because our dicts happened to serialize the same. We should use some OrderedDict instances to ensure that this does NOT happen by default, which will give us more confidence in the future.

This is pretty low priority, but worth listing as an issue since we specifically had this bite us.

Use minimum rather than pinned dependencies

Sideboard currently has a requirements.txt file
(https://github.com/magfest/sideboard/blob/master/requirements.txt)
which pins all of its dependencies. I've learned over time that a lot of Python packaging utilities (in particular setuptools and pkg_resources) enforce these dependencies.

This causes problems in the following situation:

  • Sideboard pins a default version of a library, e.g. SQLAlchemy==1.0.0.
  • Sideboard plugin pins a dependency at a higher version, e.g. SQLAlchemy==1.0.12.
  • Everything installs, but then when we try to run a command like sep, we get an error in something like pkg_resources.load_entry_point because it iterates through the requirements.txt file and finds a mismatch.

In theory we could monkeypatch pkg_resources and maybe whatever part of setuptools are enforcing this restriction.

However, I've been doing a lot of Googling on this, and the consensus seems to be, "pinning dependencies is good for applications and bad for libraries". In other words, "this is a feature not a bug" and Sideboard should instead be enforcing minimum dependencies.

I'm going to experiment to see if this causes a different set of issues. I'm concerned about the following situation:

  • Sideboard specifies a minimum dependency such as SQLAlchemy>=1.0.0.
  • Sideboard plugin pins a dependency at a higher version, e.g. SQLAlchemy==1.0.12.
  • We run python setup.py develop on Sideboard itself and install an even later version, e.g. SQLAlchemy 1.0.15.
  • We run python setup.py develop on the plugin, which pins an earlier version, which will hopefully Just Work but we'll see.

Assuming this general approach proves valid, I'll update Sideboard's requirements.txt file to change all of the dependencies from == to >=. I'll run the unit tests for Sideboard and also for all the plugins I've got here at my work. I don't expect this to break anything, but I'll let you know if there seems to be any potential to that prior to opening the PR.

Pluggable authentication

Moved here from the original (now private) issue tracker.

Sideboard was originally intended to ship with an LDAP-based authentication approach wherein plugins can use the @all_renderable class decorator and specify that its methods are "restricted". HTTP requests made to a restricted resource check whether the user is logged in and redirects them to /login if they aren't, e.g.

@all_renderable(restricted=True)
class Root(object):
    ...

There ended up being compatibility issues with the Python LDAP module we chose, and so this was scrapped, though there's still some vestigial code referencing LDAP auth in sideboard/server.py which we need to get rid of at some point since it's not used or tested or working.

It would be nice to keep the generic approach though, and even better would be making the authentication pluggable, so that a plugin could register its own named authentication approach. Then instead of saying restricted=True we can instead say restricted='ragnarauth' and instead of being redirected to the default section, we'd be redirected to whatever login form was defined. This could also define what session variables are checked and such.

Add flake8 to test suite

As a developer, I want a machine to double check my work to ensure that I am committing quality code.

From the flake8 manpage:

flake8 is a command-line utility for enforcing style consistency across Python projects. By default it includes lint checks provided by the PyFlakes project, PEP-0008 inspired style checks provided by the PyCodeStyle project, and McCabe complexity checking provided by the McCabe project. It will also run third-party extensions if they are found and installed.

Caller and GenericCaller should have delay option

Moved here from the original (now-private) issue tracker.

The Caller and GenericCaller classes have both a defer method and a delayed method. The latter makes use of a DaemonTask which periodically checks whether or not it's time to execute a task.

In practice we've found that we'll often instantiate callers with no intention of ever using the delayed method, which means we're spinning up an extra thread for nothing. It would be better to have a keyword argument on instantiation which says whether delayed will be a supported method. If not then calling it will raise an exception.

Better documentation link on the list_plugins page

When running make html in the sphinx directory, Sideboard builds its docs at a location that gets mounted by CherryPy. The list_plugins page has a link to this page.

Unfortunately, most developers will see this link and click it without building the docs, which means that the link is broken in a confusing way.

We should instead upload our Sphinx docs to somewhere like https://readthedocs.org/ and then change the link to point there.

Log request context when debug is turned on

Sideboard has a debug config option which toggles a couple of behaviors. For example, on exceptions we return a full stacktrace in the RPC response when debug is set, but otherwise we only return the error message.

We should add onto this and log the entire request contents, e.g. the entire RPC request when debug is set to true. This would help us out in development where a staging server gets a weird exception we don't expect and we didn't have logging turned up high enough to see the entire request.

Default model repr should omit id when other unique columns are present

Moved here from the original (now-private) issue tracker.

SQLAlchemy models in Sideboard get a default repr which by default displays all unique columns. So when going through the Sideboard tutorial, you'll see something that looks like

[<Website id='9d4eb9f2-3b92-4ddf-9b22-999b2063d98b' url=u'http://hastheworldendedyet.com'>, <Website id='cc06e57b-eb3d-484c-908c-bb4f97b81306' url=u'http://hasthelargehadroncolliderdestroyedtheworldyet.com'>]

This would be far more legible if it just read

[<Website url=u'http://hastheworldendedyet.com'>, <Website url=u'http://hasthelargehadroncolliderdestroyedtheworldyet.com'>]

My opinion is that we probably don't want the id to be present in the default repr when there are already other unique columns.

Rename "client" to "subscription" in WebSocket RPC messages

As explained in these and other Sideboard docs, our WebSocket RPC messages have a "callback id" which identifies specific calls and and a "client id" which identifies a subscription. It's become clear to me from explaining this to people that "client id" is a terrible name for the id of a subscription, and it should really be "subscription id".

We can make this change in a backwards-compatible way:

  • Update the Sideboard server code to check for both a subscription and a client field, defaulting to subscription.
  • Update the Sideboard client code to set both fields, since Sideboard ignores extra fields and thus it won't hurt to have the redundancy.
  • Eventually after all deploys have been updated, remove the client field entirely.

Decide what new SQLAlchemy features should be rolled into Sideboard

MAGFest implements a ton of SQLAlchemy utilities, including a lot of Django idioms we ported over from older versions of our codebase. Some of these might be generally useful for Sideboard, others should probably be left up to plugins to define for themselves.

For the sake of having a record of these things, I'm going to keep a running list in this ticket. We can split some of these out into separate issues as we decide to tackle them.

  1. Sideboard ships with a UTCDateTime class, and our documentation explains that you can just set a client-side default, e.g.
created = Column(UTCDateTime, default=lambda: datetime.now(UTC))``

The problem with this approach is that the timestamp is set when the model is instantiated rather than when it's saved. There's no easy way to set a server-side default. SQLAlchemy actually documents an approach for this (http://docs.sqlalchemy.org/en/rel_0_9/core/compiler.html#utc-timestamp-function), in the same way that itertools has a ton of recipes in their docs without actually including them in the itertools module itself which we can apply like so:

class utcnow(FunctionElement):
    type = UTCDateTime()

@compiles(utcnow, 'postgresql')
def pg_utcnow(element, compiler, **kw):
    return "timezone('utc', current_timestamp)"

@compiles(utcnow, 'sqlite')
def sqlite_utcnow(element, compiler, **kw):
    return "(datetime('now', 'utc'))"

We might want to extend this further and include it in Sideboard. Or not, since plugins might not care whether it's a client default or a server default.

  1. We used to have a lot of columns which were defined in Django like
shirt = IntegerField(choices=SHIRT_OPTS, default=NO_SHIRT)

And this seems like a reasonable thing to want to do; in our original app we sometimes implemented this as (blech) a plain old UnicodeText column and just didn't allow the UI code to set a bad value. This seems bad, so maybe we want to include a Choice type; in the MAGFest code I just say

shirt = Column(Choice(c.SHIRT_OPTS), default=c.NO_SHIRT)

so if we want to then we could roll the Choice type into Sideboard.

  1. This one is probably a more specific to MAGFest, but I have a lot of columns which represent checkbox groups. I previously used the CommaSeparatedIntegerField in Django for this, so in the MAGFest code now I have code like
assigned_depts = Column(MultiChoice(c.DEPARTMENT_OPTS))

I have no idea if this is something that would be generally useful, so it's probably not a good candidate for inclusion in Sideboard at this time. If we end up writing more plugins that could use this then maybe we can roll it in.

  1. SQLAlchemy models by default do not implement the __hash__ method, which I would always want in any of my apps. It also doesn't implement a sensible __eq__ so we should probably roll that into what our @declarative_base provides.

  2. It should always be easy to look up an object by id, since that's the most common thing I think I do in any of my ORM code. The default way to do that in Sideboard is e.g.

with Session() as session:
    attendee = session.query(Attendee).filter_by(id=id).one()

Which is okay but a little verbose. I'd like to be able to just say

with Session() as session:
    attendee = session.attendee(id)

Which I'm doing in the MAGFest code by adding a lookup method to the session for each model which takes an id. We might want this in Sideboard core, or maybe it's too magicky even for Sideboard.

  1. When dealing with an object, it's often useful to get the session which that object is attached to. Otherwise, when passing a model object to a function, you have to pass both it and the session object. Fortunately, you can use the object_session method which SQLAlchemy provides (which just returns None if the object is unattached) to do this, so I implemented this in the MAGFest code:
    @property
    def session(self):
        return Session.session_factory.object_session(self)
  1. This is related to 2 above, but Django has a get_FIELD_display option, so that if you have a choice column named shirt, you can say attendee.get_shirt_display() to get the textual description. I faked this with some code in __getattr__ in my base class, but it might do to make this more generic. Or not - we'll see how often we end up wanting to do something like this.

  2. sideboard.tests.patch_session creates a new session_factory, which means that the SQLAlchemy ORM events which were defined will still work, but the SQLAlchemy core events will not. For example, MAGFest defines before_flush and after_flush events. We could make these automatically get listened-for on the patched session object. For now I'm just going to do it manually, and until we implement this, we should just document this limitation.

  3. In the MAGFest tests, I use the same idiom we use in our Sideboard tests for testing database interactions: generate a test.db SQLite file once, then basically just do a cp to restore it on every test. Since this is something I want to happen for every test module, it makes sense to put this fixture in conftest.py, since that will make it available everywhere. (If you specify autouse=True for a fixture in a module, it's still only auto-used within that module or other modules its imported into, but if you specify a fixture with autouse=True in conftest.py it's available and automatically used everywhere.)

So the conftest.py we'd add would look something like this:

import shutil
import pytest
from sideboard.tests import patch_session
from {{ module }} import sa

@pytest.fixture(scope='session', autouse=True)
def init_db(request):
    patch_session(sa.Session, request)
    with sa.Session() as session:
       pass  # make insertions here

@pytest.fixture(autouse=True)
def db(request, init_db):
    shutil.copy('/tmp/test.db', '/tmp/test.db.backup')
    request.addfinalizer(lambda: shutil.copy('/tmp/test.db.backup', '/tmp/test.db'))

We'd also need some documentation explaining why we're doing this and how to use it effectively.

  1. This is almost certainly not something we'd want to generally do; this seems more like a "weird Eli idiom" than anything, and it almost certainly would not apply to a rich Javascript application, which is probably what most people will be writing anyway.

I have a ton of page handlers which render templates. When someone submits the form, these typically run validations and then either return the rendered form again with an error message or save and redirect somewhere with a success message.

So in order to avoid saying with Session() as session at the top of every function, I use my all-time favorite thing (a class decorator) to automatically apply a sessionized decorator to each method, which checks for the presence of a session argument and then does the with Session() as session part automatically and passes that. Since I typically redirect on success and render on error, the decorator codifies that convention:

def _get_innermost(func):
    return _get_innermost(func.__wrapped__) if hasattr(func, '__wrapped__') else func

def sessionized(func):
    @wraps(func)
    def with_session(*args, **kwargs):
        innermost = _get_innermost(func)
        if 'session' not in inspect.getfullargspec(innermost).args:
            return func(*args, **kwargs)
        else:
            with Session() as session:
                try:
                    retval = func(*args, session=session, **kwargs)
                    session.expunge_all()
                    return retval
                except HTTPRedirect:
                    session.commit()
                    raise
    return with_session

This doesn't seem worth including in Sideboard itself, but I figured I'd mention it here since it's a thing I'm doing with the Sideboard code. (Rob Ruana might argue that this implies that our with Session() as session idiom is considered harmful since I'm writing decorators to work around it. I don't agree, but that seemed like another reason to document what I'm doing here.)

  1. I'm doing a customized-for-MAGFest version of something that we might want to be able to turn on a generic version of for any Sideboard plugin with a database. Basically, for every database module, I want to be able to track all changes made to each row over time; when and how it was added, updated, and deleted, as well as who it was who made the changes.

So the question is whether a generic version of this would be generally applicable. Given how much "audit logging" we've done on a lot of our apps, this seems like a good thing. On the other hand, the specifics might well be so different between projects that it's not worth having a base implementation if everyone will have to customize it so heavily they might as well roll one from scratch.

Refactor docstrings to use Napoleon syntax

Moved here from the original (now private) issue tracker.

We have a lot of docstrings (though we should really add more), and I really like the formatting examples for https://pypi.python.org/pypi/sphinxcontrib-napoleon so I think we should start using that. Seriously, check out how much nicer it looks than ReStructuredText.

This would also be a really good opportunity for a newcomer to div into the Sideboard codebase and see what it's doing, since it'll force you to read through and understand the docs!

update debug server lockfile delete function to be more elegant

As it happens, Sideboard's config already has a root setting which is the top-level directory (and a module_root setting which is the top-level directory of the settings).

Although really, since we have a CherryPy setting for the session location, you should really use that. Also, the glob function is your friend here. So you could rewrite this function to read:

import os
from glob import glob

from sideboard.lib import config

def remove_session_lockfiles():
    for lockfile in glob(os.path.join(config['cherrypy']['tools.sessions.storage_path'], '*.lock')):
        os.remove(lockfile)

(see discussion in https://github.com/magfest/sideboard/pull/6/files)

thread name setting in linux is broken

maybe. just noticed our thread names are not showing up in htop.

we have a linux syscall on thread start that attempts to set the name, maybe that's no longer happening for some reason, check

low priorirty

plugin loading: sort glob() output alphabetically by basename

See #13 (comment) for a discussion on why this is a good idea.

This line here in imports.py:
plugin_dirs = [d for d in glob(join(config['plugins_dir'], '*')) if isdir(d) and not basename(d).startswith('_')]

returns a list of plugin directories it finds, sorted in no particular order (glob() is unsorted)

Sideboard later prioritizes some of the items in this list to ensure a proper load order, BUT, the unprioritized ones can be loaded in a different order in different systems.

In THEORY this never matters, but, if we sorted the unprioritized plugins alphabetically, then the load order will be the same on all systems and reduce the chance that we could ever see weird system-specific bugs that appear to only happen on one machine and not the other.

This is kind of low priority and me being paranoid and overkill BUT still a useful and simple thing we can do to decrease the chance of encountering subtle heisenberg bugs later on.

500 error loading /list_plugins

Running master branch, out of the box:

500 Internal Server Error

The server encountered an unexpected condition which prevented it from fulfilling the request.

Traceback (most recent call last):
  File "/app/env/lib/python3.4/site-packages/cherrypy/_cprequest.py", line 631, in respond
    self._do_respond(path_info)
  File "/app/env/lib/python3.4/site-packages/cherrypy/_cprequest.py", line 690, in _do_respond
    response.body = self.handler()
  File "/app/env/lib/python3.4/site-packages/cherrypy/lib/encoding.py", line 221, in __call__
    self.body = self.oldhandler(*args, **kwargs)
  File "/app/env/lib/python3.4/site-packages/cherrypy/_cpdispatch.py", line 60, in __call__
    return self.callable(*self.args, **self.kwargs)
  File "/app/sideboard/lib/_cp.py", line 162, in renderer
    output = method(self, *args, **kwargs)
  File "/app/sideboard/server.py", line 75, in list_plugins
    plugin_info[plugin]['paths'].append(path)
KeyError: 'sideboard'

Refactor tags support out of sideboard.lib.sa

Moved here from the (now private) original Sideboard issue tracker.

One of our early apps had some special casing for a "tags" attribute in its models. There's one specific place in the code where we check for tags and handle it appropriately.

We don't want to hard-code this into core Sideboard; at the same time, we want that code to work as a Sideboard plugin. So for now I think we can just cut this out of our code, but we should spend just a few minutes thinking about how to make this code extensible, perhaps adding support for the concept of "foreign key into any table" which is really what tags are all about.

Modernize sideboard build tools

Eight years ago the python packaging landscape was in disarray. There was setuptools, distribute, distutils, distutils2, paver, and pip – with no clear standard on how to build and package python projects. Wheels didn't exist yet, and virtualenv was still new.

Sideboard makes use of paver (and uber still has a bootstrapped distribute_setup.py file), but this is really a vestige of a time long past. We should probably remove pavement.py and replace any pavement tasks that we currently use with console_scripts in setup.py.

This doesn't actually create any value for us – other than presenting a more familiar tool-chain for new developers – so this issue should be a very low-priority. But I wanted to capture it for future reference.

Developers get a confusing warning about Sphinx docs

I ssh'd onto my Ubuntu server. Cloned this repo, ran paver make_venv followed by paver install_deps and when I use ./env/bin/python sideboard/run_server.py I get a code-hang with the text:
CherryPy Checker: u'/root/sideboard/sideboard/docs/html' (root + dir) is not an existing filesystem path. section: [/docs] root: None dir: u'/root/sideboard/sideboard/docs/html'

Package sideboard and upload it to pypi.python.org

Just an idea I'd like to throw out there. There are some scenarios – potentially for testing plugins – where it would be nice to have a packaged version of sideboard on pypi.python.org.

This isn't immediately valuable to MAGFest, so it should be a very low-priority. But I thought it would be worth capturing for future reference.

paver create_plugin should generate a pep8-compliant project skeleton

Our current paver create_plugin command generates files with some non-pep8-compliant whitespace. Specifically there are some places where we don't put 2 lines before class/function definitions.

This is somewhat harder to fix than it sounds. There are a bunch of places where we do things like

{% if GENERATE_WEBAPP %}
@all_renderable()
class Root:
    ...

Generating newlines such that we end up with the correct number for all combinations of settings will take a careful eye and a bit of testing, but overall shouldn't be too bad.

Plugins should standardize their development sqlite locations

Moved here from the original (now private) issue tracker.

Sideboard has a db directory with a README file which explains "Directory for plugins' sqlite files. Sideboard exposes this directory as a config option called 'sqlite_dir'."

However, our default development-defaults.ini file for plugins actually puts the file at /tmp/PLUGIN_NAME.db which is also fine, but doesn't agree with the above.

Also, some of our unit tests put the test sqlite file in PLUGIN_ROOT/data which is also fine, but doesn't agree with either of the above.

We should make all three of these things the same. I don't really care which, so long as the current production config remains unchanged (/opt/sideboard/db).

Add a SIDEBOARD_CONFIG_OVERRIDES environment variable to specify config file paths

I believe @EliAndrewC has done some work on this already in an upstream (downstream?) fork. I don't know how the current implementation works, so this is a proposal of how I think it should work. As always, I welcome comments and suggestions!

Proposal

Upon startup, sideboard will read an environment variable SIDEBOARD_CONFIG_OVERRIDES to determine where it should look for custom config files. SIDEBOARD_CONFIG_OVERRIDES will contain a semicolon separated list of absolute and relative paths to config files.

SIDEBOARD_CONFIG_OVERRIDES="/absolute/path/config.ini;relative/path/config.ini"

Implementation

In all cases, sideboard will first look for config files in the following locations, in order:

  • /etc/sideboard/sideboard-core.cfg
  • /etc/sideboard/sideboard-server.cfg
  • /etc/sideboard/plugins.d/<PLUGIN_NAME>.cfg for each loaded plugin

If SIDEBOARD_CONFIG_OVERRIDES is empty or does not exist in the environment, then no further configuration is performed.

Otherwise, sideboard will look for each config file specified by SIDEBOARD_CONFIG_OVERRIDES.
If an absolute path is used, sideboard will only load the specified config file once.
If a relative path is used, then sideboard will search for the config file relative to the sideboard directory, and again relative to each plugin directory.

If any config files are specified with a <FILENAME>-defaults.<EXT> suffix, then sideboard will also look for a similarly named file without the -defaults suffix, <FILENAME>.<EXT>. So development-defaults.ini will also attempt to load development.ini, and test-defaults.ini will also attempt to load test.ini.

In no case is a missing config file considered an error. If any of the specified config files do not exist, sideboard should issue a WARNING (maybe an INFO?) and continue without error.

Examples

Ex: SIDEBOARD_CONFIG_OVERRIDES not specified in environment

Running sideboard with uber and reports plugins, and SIDEBOARD_CONFIG_OVERRIDES is not specified in the environment.

The following config files will be loaded, if they exist, in order:

  • /etc/sideboard/sideboard-core.cfg
  • /etc/sideboard/sideboard-server.cfg
  • /etc/sideboard/plugins.d/uber.cfg
  • /etc/sideboard/plugins.d/reports.cfg

Ex: Using a single absolute path

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="/home/robruana/sideboard/development.ini"

The following config files will be loaded, if they exist, in order:

  • /etc/sideboard/sideboard-core.cfg
  • /etc/sideboard/sideboard-server.cfg
  • /etc/sideboard/plugins.d/uber.cfg
  • /etc/sideboard/plugins.d/reports.cfg
  • /home/robruana/sideboard/development.ini

Ex: Using a single relative path

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="development.ini"

The following config files will be loaded, if they exist, in order:

  • /etc/sideboard/sideboard-core.cfg
  • /etc/sideboard/sideboard-server.cfg
  • /etc/sideboard/plugins.d/uber.cfg
  • /etc/sideboard/plugins.d/reports.cfg
  • <CURRENT_DIR>/development.ini
  • <CURRENT_DIR>/plugins/uber/development.ini
  • <CURRENT_DIR>/plugins/reports/development.ini

Ex: Using a relative path with -defaults suffix

Running sideboard with uber and reports plugins and:

SIDEBOARD_CONFIG_OVERRIDES="test-defaults.ini"

The following config files will be loaded, if they exist, in order:

  • /etc/sideboard/sideboard-core.cfg
  • /etc/sideboard/sideboard-server.cfg
  • /etc/sideboard/plugins.d/uber.cfg
  • /etc/sideboard/plugins.d/reports.cfg
  • <CURRENT_DIR>/test-defaults.ini
  • <CURRENT_DIR>/test.ini
  • <CURRENT_DIR>/plugins/uber/test-defaults.ini
  • <CURRENT_DIR>/plugins/uber/test.ini
  • <CURRENT_DIR>/plugins/reports/test-defaults.ini
  • <CURRENT_DIR>/plugins/reports/test.ini

Rationale

Why load /etc/sideboard/*.cfg first?

This is a common pattern in almost all command line and server software. Consider good old bash, first it loads the system-wide /etc/profile before loading your personalized ~/.bash_profile (there's a lot more going on here, but that's the simple explanation).

The system-wide settings in /etc should provide sane defaults. The files under /etc are loaded first so they can be overridden by subsequent files.

Why a special case for -defaults suffix?

This facilitates a common development pattern in which development-defaults.ini is checked into a git repository, while development.ini is added to .gitignore. This provides developers with a set of sane defaults, while allowing them to customize their dev environment without cluttering the git repo.

Similarly test-defaults.ini and test.ini are often used to configure a test environment.

Adding a special case to handle the -defaults suffix makes setup easier by favoring convention over configuration.

Why treat relative paths differently?

Again, by favoring convention over configuration we can save a lot of effort. Sideboard and each of it's plugins use the development-defaults.ini pattern. By searching for paths relative to the sideboard directory AND each plugin directory, we can load each of their config files by specifying development-defaults.ini only once.

Why not use a command line switch?

We certainly could! A command line switch to specify config files could work in conjunction with the environment variable to further override default settings. So a command line switch is a possible future addition, but isn't necessary at the moment.

Why do this at all?

The motivation for this proposal is to make it easier to load settings from test-defaults.ini when running unit tests. A test environment usually requires a configuration that is different from a development environment.

During development, you usually want to your server to listen on a known port and auto-reload when you make code changes. You usually want to use the same database that you use in production (like Postgres).

development-defaults.ini

[cherrypy]
engine.autoreload.on = True
server.socket_port = 8282

[secret]
sqlalchemy_url = "postgresql://rams_db:rams_db@localhost:5432/rams_db"

When running unit tests, you DON'T want your server to auto-reload when you make code changes. In order to avoid address conflicts you DON'T want to listen on a known port. It's common to run unit tests against a sqlite database, because tests may repeatedly create and destroy a database. (For functional tests, you probably still want the same database that you use in production)

test-defaults.ini

[cherrypy]
engine.autoreload.on = False
server.socket_port = 0

[secret]
sqlalchemy_url = "sqlite+pysqlite:///file.db"

Without any way to specify a different test configuration, developers must dynamically patch development-defaults.ini in code.

broken sort order for plugins, priority_plugins not honored

This one's fun :) This is one of those bugs where it's literally been busted for years but has by certain system-dependent miracles worked correctly and we never noticed. I need to double check this, but I'm now pretty confident this is real.

TLDR: sideboard doesn't honor the order of priority_plugins

"BUT wait!" I hear you say. "If that were true, it should have been crashing for years, right? How did we never notice??"

We never noticed it wasn't working because on literally every box we've ever run, glob() (which returns an unordered result) happened by PURE CHANCE to return a plugin load order that happened to work despite our sorting not working.

  1. glob() ordering is arbitrary, it is not sorted in any way:
    "By checking the source code of glob.glob you see that it internally calls os.listdir, described here:
    http://docs.python.org/library/os.html?highlight=os.listdir#os.listdir
    Key sentence: os.listdir(path) Return a list containing the names of the entries in the directory given by path. The list is in arbitrary order. It does not include the special entries '.' and '..' even if they are present in the directory."

  2. that non-ordering shouldn't be a problem because our code is supposed to re-order things so the prioritized plugins get loaded first.

but that's not what happens, here's the result of an instrumented build which spits out some debug info about this process (https://github.com/magfest/sideboard/blob/ordering_testing_NEVERUSE/sideboard/internal/imports.py)

glob() results on Vicki's machine: returns a BROKEN LOAD ORDERING

priority_plugins: ['panels', 'uber'] 
glob("/home/vagrant/uber/sideboard/plugins/*") says:
['/home/vagrant/uber/sideboard/plugins/uber_analytics',
'/home/vagrant/uber/sideboard/plugins/hotel',
'/home/vagrant/uber/sideboard/plugins/barcode',
'/home/vagrant/uber/sideboard/plugins/magclassic',
'/home/vagrant/uber/sideboard/plugins/uber',
'/home/vagrant/uber/sideboard/plugins/tabletop',
'/home/vagrant/uber/sideboard/plugins/bands',
'/home/vagrant/uber/sideboard/plugins/attendee_tournaments',
'/home/vagrant/uber/sideboard/plugins/panels'

would load plugins in the following order:
['uber_analytics', 'hotel', 'barcode', 'magclassic', 'uber', 'tabletop', 'bands', 'attendee_tournaments', 'panels']

NOTE: BROKEN: panels needs to load before tabletop, else tabletop crashes startup

same test but run on staging2 with identical set of plugins and priority_plugins results in a DIFFERENT LOAD ORDER (BUG)

priority_plugins: ['panels', 'uber']

glob("/usr/local/uber/plugins/*") says:
['/usr/local/uber/plugins/barcode',
'/usr/local/uber/plugins/magclassic',
'/usr/local/uber/plugins/attendee_tournaments',
'/usr/local/uber/plugins/uber_analytics',
'/usr/local/uber/plugins/panels',
'/usr/local/uber/plugins/tabletop',
 '/usr/local/uber/plugins/bands',
'/usr/local/uber/plugins/uber',
'/usr/local/uber/plugins/hotel']

would load plugins in the following order:
['barcode', 'magclassic', 'attendee_tournaments', 'uber_analytics', 'panels', 'tabletop', 'bands', 'uber', 'hotel']

That load order is a valid order.


solution: uber's code should be handling this case correctly on Vicki's machine but isn't, so it's a legit bug we need to fix. Should be an easy fix, and we should probably unit test the hell out of this to make sure it loads things correctly.

Crud._distinct_query should work in backends other than postgresql

Moved here from the (now private) original Sideboard issue tracker.

The SQLAlchemy
http://docs.sqlalchemy.org/en/rel_0_8/orm/query.html#sqlalchemy.orm.query.Query.distinct
method only works on specific columns with Postgresql. This means that there's a significant difference between the two forms of distinct that sqlalchemy supports:

>>> [tag.name for tag in session.query(Tag).all()]
[u'Male', u'Male', u'Ninja', u'Pirate']
>>> session.query(Tag).distinct(Tag.name).count()
4
>>> session.query(Tag.name).distinct().count()
3

This is probably fine as long as we document this; I'd generally recommend using postgres everywhere, but it would be nice to not have this restriction. This would require a small but non-trivial rewrite to how we're performing these queries, and we should un-skip the relevant tests.

Logging prefix for call chains on a distributed system

Moved here from the original (now private) issue tracker.

As a developer trying to debug a problem in production, I'd like a common logging prefix to be shown on all related requests across multiple servers, to help me isolate relevant log messages.

Let's say a user on example.com makes a websocket request to foo.bar. Behind the scenes, that method makes a separate request to baz.baf, which is a remote service running on example.org, so we make an websocket RPC request.

When debugging a problem, it can be tricky and annoying to link the requests on different servers to one another. Since all logging goes through sideboard.lib.log, it would be pretty easy to generate a random logging token and prepend that to our log messages. When making any outgoing request, we'd add a log_token field to the websocket JSON, which would be used if present.

Better detection and error messages for clock differences

Moved here from the original (now private) issue tracker.

We sometimes run into the following situation:

  • client and server have vastly different clock times
  • client logs in
  • server sends back a cookie with an expiration date which according to the client's clock is in the past
  • client web browser discards the cookie as expired and doesn't send it back to the server on the next request
  • server sees that there's no session cookie, interprets the client as not being logged in, redirects client to the login page

This is basically impossible to prevent, but it would be nice if we detected when it looked like it was happening, say by detecting anytime we set a cookie and then get back no cookie on the next request, or something.

Better error message on infinite redirect

We occasionally end up with a misconfigured system which causes the following error:

  • default_url is set to something like /foo
  • a user visits / and is redirected to /foo
  • /foo doesn't exist, so we get redirected to our default url, which is /foo
  • the process repeats until the browser eventually gives a "too many redirects" error

This is confusing for the person trying to debug. What we should do instead is that in sideboard/server.py where we raise the HTTPRedirect back to our default URL, we should check to see whether we're already in our default URL; if so then we should instead return a 404 with a more helpful error message, e.g. "default URL /foo doesn't exist".

Test server should use an ephemeral port to avoid "address already in use" errors

As a developer, one of the first things I like to do when I clone a project repo is run the test suite. Ideally I should be able to run tox at the command line and everything works.

The tests in sideboard/tests/test_server.py run a server that always binds to port 8282. If that port is already in use, the result is a bunch of "address already in use" errors.

Instead, the server should bind to port 0 to request an already open ephemeral port. The socket can then be inspected to determine what port it's listening on.

Call stack logs should be emitted at TRACE level

Moved here from the (now private) original Sideboard issue tracker.

One of the cool things that pytest does is when an exception occurs in a unit test, it can spit out a complete call stack. Not just a stack trace, but even all of the local variables that exist at every step of the stack.

This is probably something we can pull out of pytest and use for Sideboard exceptions. This is WAY too verbose to be turned on by default, not to mention dangerous in terms of possibly logging passwords in memory and such. But it would be super-useful to have the option. One way to do this would be to do the logging at TRACE level so that even DEBUG wouldn't get it.

Even if we can't pull it out of pytest, it wouldn't be that hard to implement using the same introspection tools they're using. The trick would be making it compatible with both Python 2 and 3 and ideally turning it off for non-CPython interpreters such as Jython or IronPython where sys._getframe might not be available.

Move "PROJECT_DIR/sideboard/tests" package to "PROJECT_DIR/tests"

This is largely a personal preference, and there are pros and cons to both directory structures.

Hopefully we can get the best of both worlds with the following configuration:

  • PROJECT_DIR/sideboard/tests will contain utilities to facilitate test creation by both sideboard developers and plugin developers
  • PROJECT_DIR/tests will contain the actual sideboard unit/functional tests

Since I am going to be working on tests anyway, I figured I may as well do this while I'm at it.

Add a "delay" parameter to DaemonTask

It would be nice if you could specify an initial amount of time that a DaemonTask should wait before running its task the first time. This would help stagger tasks when the system initially starts up.

Run Server alias fails on working install

Hey Eli, looking for some help here. I'm working on getting sideboard functional for a school project.

(env) vagrant@vagrant-ubuntu-trusty-64:~/uber/sideboard$ run_server
Traceback (most recent call last):
  File "/home/vagrant/uber/sideboard/sideboard/run_server.py", line 5, in <module>
    import sideboard.server
  File "/home/vagrant/uber/sideboard/sideboard/server.py", line 207, in <module>
    cherrypy.tree.mount(Root(), '', app_config)
  File "/home/vagrant/uber/sideboard/sideboard/server.py", line 202, in mount
    assert script_name not in cherrypy.tree.apps, '{} has already been mounted, probably by another plugin'.format(script_name)
AssertionError:  has already been mounted, probably by another plugin

This error pops up whenever I execute run_server.py.

I've had a few other errors, but I've been working my way around them.

EDIT:

More info:
This is the state of cherrypy.tree.apps when the assert fails.

{u'': cherrypy._cptree.Application(<sideboard.server.Root object at 0x7fcda7c06190>, u'')}

and it appears that the script_name its searching for is hardcoded to be ""

/wsrpc should use the client cert username as the session username

Moved here from the original (now private) issue tracker.

/wsrpc is assumed to be client-cert protected. Currently the "username" we log for those requests is rpc, but it would be nice to use this as the default, but if a client cert exists with a non-empty CN then we'd use that as the username.

This currently only matters for logging, but this will be relevant when we expose different authentication and authorization methods. In particular, it would be nice to restrict some API calls to only be callable by certain clients.

Foreign key tests are skipped due to Jenkins' sqlite packages

Moved here from the (now private) original Sideboard issue tracker.

We currently have two tests for foreign key support where we make sure that deleting a row (especially over our CRUD API) cascades. This works on my machine but fails on some other machines, and some basic Googling shows that foreign key support is not always compiled into SQLite. Depending on your package your foreign key declarations in your CREATE TABLE statements may just be ignored in SQLite.

We should do two things:

  • confirm whether that's true before doing any work on this ticket, since I didn't spend enough time to be certain of what I was reading
  • detect whether SQLite has foreign key support compiled in, and only skip() if it isn't via the skipif decorator

Allow port to be configurable via test.ini

As of #59, the server.socket_port was hardcoded to 0, but now that we've fixed #66, we can use test-defaults.ini and test.ini to configure it.

I propose keeping the original test port (8282) in test-defaults.ini, along with a note that it can be overridden and set to 0 in test.ini.

There is a rare possibility that the tests can fail when using port 0 (which has happened twice on Travis since #59), so in general using port 8282 will be more stable. However, I'm almost always using port 8282 on my dev box, so I definitely want to be able to override it locally.

Sporadic error in TestWebsocketSubscriptions.test_client_locking

I've only seen it happen in python 2.7, and it only happens sporadically. Usually goes away if you re-run the build.

py27 runtests: commands[0] | coverage run --source sideboard -m py.test
============================= test session starts ==============================
platform linux2 -- Python 2.7.12, pytest-3.0.7, py-1.4.33, pluggy-0.4.0
rootdir: /home/travis/build/magfest/sideboard, inifile: setup.cfg
collected 283 items 
sideboard/tests/test_configuration.py ..............................
sideboard/tests/test_imports.py .
sideboard/tests/test_jsonrpc.py ..........
sideboard/tests/test_lib.py .................................................
sideboard/tests/test_logging.py .
sideboard/tests/test_sa.py ..........s...........s...........................s...............................................
sideboard/tests/test_sep.py ....
sideboard/tests/test_server.py ....F..........................
sideboard/tests/test_websocket.py ..................
sideboard/tests/test_websocket_dispatcher.py .........................................
=================================== FAILURES ===================================
________________ TestWebsocketSubscriptions.test_client_locking ________________
self = <sideboard.tests.test_server.TestWebsocketSubscriptions testMethod=test_client_locking>
    def test_client_locking(self):
        self.ws._send(method='self.slow_echo', params=['foo'],
                          client=self.client, callback='cb1')
        sleep(1)
        self.ws._send(method='self.echo', params=['bar'],
                          client=self.client, callback='cb2')
>       self.assert_incoming(data='foo', timeout=2)
sideboard/tests/test_server.py:391: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
self = <sideboard.tests.test_server.TestWebsocketSubscriptions testMethod=test_client_locking>
ws = None, client = None, timeout = 2, params = {'data': 'foo'}
data = {'_time': 1.0030920505523682, 'callback': 'cb2', 'client': 'test_client_locking', 'data': 'bar'}
@py_assert1 = False, @py_assert0 = None, @py_assert5 = None, @py_assert12 = None
@py_assert14 = None, @py_assert16 = None
    def assert_incoming(self, ws=None, client=None, timeout=1, **params):
        data = self.next(ws, timeout)
        assert (client or self.client) == data.get('client')
        for key, val in params.items():
>           assert val == data[key]
E           AssertionError: assert 'foo' == 'bar'
E             - foo
E             + bar
sideboard/tests/test_server.py:167: AssertionError
=============== 1 failed, 279 passed, 3 skipped in 79.70 seconds ===============
ERROR: InvocationError: '/home/travis/build/magfest/sideboard/.tox/py27/bin/coverage run --source sideboard -m py.test'
___________________________________ summary ____________________________________
ERROR:   py27: commands failed
The command "tox -e $TOX_ENV" exited with 1.
Done. Your build exited with 1.

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 100 times too long! We should probably be waiting at most 1 millisecond, since on a fast local network I'd expect to have response times in the 1s of milliseconds (this is born out by recent experiments).

I don't see any value in making this a configurable option, but we should update it to use a 1ms wait instead of a 100ms wait.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.