Issue 2683 - Use Rietveld "adblockplus" branch with custom settings.py

Issue 2683 - Use Rietveld "adblockplus" branch with custom settings.py

[Wladimir Palant, 2015-06-23 12:44:43]

Creating a Rietveld branch shouldn't be necessary. GAE sandboxes allow
overriding specific module properties. Required a bit of experimenting but the
following should work:

  from google.appengine.tools.devappserver2.python.sandbox import
_MODULE_OVERRIDE_POLICIES
  class SettingsPolicy(object):
    def apply_policy(self, module_dict):
      settings = module_dict['LazySettings']
      orig_getattr = settings.__getattr__
      def getattr(self, name):
        if name == 'RIETVELD_INCOMING_MAIL_ADDRESS':
          return None
        else:
          return orig_getattr(self, name)
      settings.__getattr__ = getattr
  _MODULE_OVERRIDE_POLICIES['django.conf'] = SettingsPolicy()

[Wladimir Palant, 2015-06-23 13:18:02]

Sorry, much simpler than that, no need to mess with Django:

  from google.appengine.tools.devappserver2.python.sandbox import
_MODULE_OVERRIDE_POLICIES
  class SettingsPolicy(object):
    def apply_policy(self, module_dict):
      module_dict['RIETVELD_INCOMING_MAIL_ADDRESS'] = None
  _MODULE_OVERRIDE_POLICIES['settings'] = SettingsPolicy()

This needs to run in our wrapper of course.

[Wladimir Palant, 2015-06-23 13:25:48]

Or even with the built-in policy:

  from google.appengine.tools.devappserver2.python.sandbox import
_MODULE_OVERRIDE_POLICIES, ModuleOverridePolicy
  _MODULE_OVERRIDE_POLICIES['settings'] = ModuleOverridePolicy(
      overrides={'UPLOAD_PY_SOURCE': 'test'},
      default_pass_through=True
  )

[mathias, 2015-06-29 07:27:42]

On 2015/06/23 13:25:48, Wladimir Palant wrote:
> Or even with the built-in policy:
> 
>   from google.appengine.tools.devappserver2.python.sandbox import
> _MODULE_OVERRIDE_POLICIES, ModuleOverridePolicy
>   _MODULE_OVERRIDE_POLICIES['settings'] = ModuleOverridePolicy(
>       overrides={'UPLOAD_PY_SOURCE': 'test'},
>       default_pass_through=True
>   )

While I do agree that this is a quite clever alternative, I'm unsure whether
this is a good approach in the long turn. In theory the `wrapper.py` stuff can
become independent from module `rietveld` one day. So one could create a
distinct, straightforward GAE module - which sounds quite appealing to me.
Integrating Rietveld-specific monkey-patches would create an unnecessary
coupling in this case.

The current approach, while less elegant, is quite manageable. Even if a few
more patches become required, e.g. if the recent help-text issue we've reported
upstream is not handled in a reasonable time. Of course we have to remain
disciplined and not start over-customizing things. We could also report this
very `settings.py` issue upstream, by the way..

Last but not least; please note that the branch is already created and tested.

However, reverting this and updating the patch-sets is not much effort. And
beside acknowledging the simplicity of the proposed solution, I do also see that
my concerns relevance stands and falls with the possible future GAE module,
which is not something we actually *need*. So, any feedback on this idea?

[Wladimir Palant, 2015-06-29 10:28:03]

On 2015/06/29 07:27:42, mathias wrote:
> In theory the `wrapper.py` stuff can
> become independent from module `rietveld` one day.

Nope, we definitely don't want to use that hack for anything other than
Rietveld. In fact, even supporting it for Rietveld isn't a great idea in the
long term - so we might want to keep looking for alternatives that don't require
such a weird setup.

> Integrating Rietveld-specific monkey-patches would create an unnecessary
> coupling in this case.

The wrapper is already Rietveld-specific, we are only patching functionality
that is used by Rietveld.

> The current approach, while less elegant, is quite manageable.

Nope, it makes updating Rietveld more complicated (and we definitely will have
to update Rietveld). We actually invested significant effort already in order to
*not* patch Rietveld.

> Last but not least; please note that the branch is already created and tested.

We can always remove it again - for me that isn't a good reason to use a
suboptimal solution.

[mathias, 2015-06-29 11:00:19]

Very well. I'll close this review in favor of a new one for the discussed
changes.