On Tue, Nov 17, 2015 at 3:18 PM, Natalia Bidart
<email address hidden> wrote:
>
>
> Diff comments:
>
>>
>> === renamed file 'Makefile' => 'Makefile.common'
>> --- Makefile  2015-11-12 17:08:24 +0000
>> +++ Makefile.common   2015-11-17 13:23:47 +0000
>> @@ -1,9 +1,9 @@
>> -# Copyright (C) 2014  Canonical Ltd.
>> -
>> +# Copyright (C) 2014 Canonical Ltd.
>
> Since you are editing this file, would you mind updating the Copyright notice to 2014-2015?

Done

>> +#
>>  CM ?= /usr/lib/config-manager/cm.py
>>  CONFIGMANAGER = config-manager.txt
>>  CONN_CHECK_CONFIG_PATH ?= /tmp/sso_conn_check_config.yaml
>> -DJANGO_SETTINGS_MODULE = settings
>> +DJANGO_SETTINGS_MODULE ?= settings
>>  ENV = $(CURDIR)/env
>>  JUJU_ENV ?= local
>>  JUJU_REPO ?= ../.juju-repo
>> @@ -42,27 +42,32 @@
>>  $(TARBALL_BUILD_DIR):
>>       @mkdir -p $(TARBALL_BUILD_DIR)
>>
>> +## run django's collectstatic command, STATIC_ROOT defaults to ./staticfiles
>>  collectstatic:
>>       # this uses a modified collectstatic command, called linkstatic, where the
>>       # --link option uses relative urls, so save space when tarballed
>>       @DJANGO_SETTINGS_MODULE=django_project.settings_build $(DJANGO_MANAGE) linkstatic --noinput --link
>>
>> +## create a pre-gzipped version of static text assets
>>  zipstatic:
>>       @echo Gzipping static asset files...
>>       @find $(STATIC_ROOT) -name \*.js -or -name \*.css -or -name \*.svg | xargs gzip -kf
>>
>> -# variation of compilemessages target that uses settings_build
>> +## variation of compilemessages target that uses settings_build
>>  compilemessages-build:
>>       @DJANGO_SETTINGS_MODULE=django_project.settings_build $(MAKE) compilemessages
>>
>>  # note: fetch-sourcedeps needs to be manually called before this will work.
>> +## build a deployment tarbal
>
> Typo here? tarball

Fixed

>>  build-tarball: install-wheels $(TARBALL_BUILD_PATH)
>>
>> +## write the bzr revno to lib/versioninfo.py
>>  version:
>>       bzr version-info --format=python > lib/versioninfo.py
>>
>>  ### Wheels ###
>>
>> +## create/reinitialise the virtualenv
>>  virtualenv:
>>       @virtualenv --clear --system-site-packages $(ENV)
>>
>>
>> === added file 'Makefile.juju'
>> --- Makefile.juju     1970-01-01 00:00:00 +0000
>> +++ Makefile.juju     2015-11-17 13:23:47 +0000
>> @@ -0,0 +1,124 @@
>> +# sso specific config
>> +CHARM_SERIES = trusty
>> +CHARM = canonical-identity-provider
>> +CHARM_BRANCH = lp:~ubuntuone-pqm-team/canonical-is-charms/canonical-identity-provider
>> +CHARM_SRC = ../sso-charm
>> +DEPLOYER_CONFIG = $(CURDIR)/django_project/deploy.yaml
>> +DEPLOYER_TARGET = sso-app-dev
>> +
>> +# generic config
>> +LOG_DIRS ?= ./logs/www-oops ./logs/schema-updates
>> +ABS_CHARM_SRC = $(shell readlink -f $(CHARM_SRC))
>> +LOCAL_JUJU_CONFIG = .local.yaml
>> +JUJU_REPOSITORY ?= $(CURDIR)/.juju-repo
>> +JUJU_HOME ?= $(HOME)/.juju
>> +JENV = $(JUJU_HOME)/environments/$(JUJU_ENV).jenv
>> +export JUJU_REPOSITORY JUJU_ENV
>> +export I_CAN_HAZ_JUJU=yes
>> +
>> +# python config, overriding the non-juju config
>> +PYTHONPATH := $(LXC_BASEDIR)/etc:$(SRC_DIR):$(LIB_DIR):$(CURDIR)
>> +export PYTHONPATH
>> +
>> +
>> +
>> +$(CHARM_SRC):
>> +     @bzr branch $(CHARM_BRANCH) $(CHARM_SRC)
>> +
>> +# | is order only dependency - means if the dir is present, we're good. no need to rebuild
>> +# https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
>> +$(JUJU_REPOSITORY): | $(CHARM_SRC)
>> +     @mkdir -p $(JUJU_REPOSITORY)/$(CHARM_SERIES)
>> +     @rm -f $(JUJU_REPOSITORY)/$(CHARM_SERIES)/$(CHARM)
>> +     @ln -sf $(ABS_CHARM_SRC) $(JUJU_REPOSITORY)/$(CHARM_SERIES)/$(CHARM)
>> +
>> +# make sure log subdirs are created with user ownership, or else they are owned by root.
>> +$(LOG_DIRS):
>> +     mkdir -p $@
>> +     sudo chown $$USER:$$USER $@
>> +
>> +$(LOCAL_JUJU_CONFIG):
>> +     @echo "$(DEPLOYER_TARGET): {services: {sso-app: {options: { user: $$USER }}}}" > $(LOCAL_JUJU_CONFIG)
>> +.INTERMEDIATE: $(LOCAL_JUJU_CONFIG)
>> +.DELETE_ON_ERROR: $(LOCAL_JUJU_CONFIG)
>> +
>> +
>> +# we need to stop gunicorn so we can relink the python in the venv
>> +gunicorn-start:
>> +     @test -f /etc/init/gunicorn.conf && sudo service gunicorn start || true
>> +gunicorn-stop:
>> +     @test -f /etc/init/gunicorn.conf && sudo service gunicorn stop || true
>> +
>> +## install dependencies
>> +dependencies: system-dependencies system-dependencies-devel gunicorn-stop virtualenv sourcedeps install-wheels-dev gunicorn-start
>> +
>> +system-dependencies: dependencies.txt
>> +     cat dependencies.txt | xargs sudo apt-get install -y
>> +
>> +system-dependencies-devel: dependencies-devel.txt
>> +     cat dependencies-devel.txt | xargs sudo apt-get install -y
>> +
>> +## install dependencies and deploy charms
>> +bootstrap: dependencies deploy
>> +
>> +$(JENV):
>> +     juju bootstrap --upload-tools
>> +
>> +clean-old-jenv:
>> +     @test -f $(JENV) && grep -q $(shell hostname -I) $(JENV) || rm -f $(JENV)
>> +
>> +## bootstrap juju env
>> +juju-bootstrap: clean-old-jenv $(JENV)
>> +
>> +## deploy juju services and setup unit and database
>> +deploy: juju-bootstrap $(JUJU_REPOSITORY) $(LOCAL_JUJU_CONFIG) | $(LOG_DIRS)
>> +     juju-deployer --local-mods -w 15 -s 0 --debug -c $(CHARM_SRC)/deploy.yaml -c $(DEPLOYER_CONFIG) -c $(LOCAL_JUJU_CONFIG) $(DEPLOYER_TARGET)
>> +     $(MAKE) setup-db localmail I_CAN_HAZ_JUJU=yes
>> +
>> +## destroy juju environment
>> +juju-destroy:
>> +     juju destroy-environment $(JUJU_ENV) --force || true
>> +     @# work around manual provider rubbishness, and remove charms
>> +     @echo Cleaning up manual provider leftovers
>> +     @sudo rm -rf /var/lib/juju/*
>> +     @sudo rm -f /etc/init/juju*
>> +     @sudo killall -q jujud || true
>> +     @sudo killall -q mongod || true
>> +     @# clean up any charm left overs
>> +     @sudo rm -f /etc/init/gunicorn*
>> +     @sudo rm -f /etc/init.d/gunicorn*
>> +     @sudo rm -rf /srv/gunicorn /srv/conn-check
>> +
>> +## destroy juju environment and rebuild
>> +redeploy: juju-destroy deploy
>> +
>> +# preserving old alias
>> +setup-db reset-db: db-setup
>> +
>> +## setup a clean database
>> +db-setup db-reset:
>> +     $(CHARM_SRC)/scripts/create-dev-db
>> +     $(MAKE) migrate
>> +
>> +## migrate the db
>> +migrate:
>> +     $(DJANGO_MANAGE) migrate --noinput
>> +
>> +## run server in foreground for debugging
>> +run: ARGS=0.0.0.0:8080
>> +run:
>
> Why do we need to change from $(ENV)/bin/gunicorn django_project.wsgi:application to -./scripts/run-gunicorn.sh?

So, the usual devel workflow would be to use the gunicorn configured
and run by the charm, which is pretty close to prod but with a few dev
tweaks (like code reloading).  So in this set up, make run is only
really for use when you want to use pdb or similar debugging tool, so
you can view/write to stdout/stdin.

For that, there are two changes, one of which prompted a separate script

1) we reuse the gunicorn config generated by the charm even when
running in the foreground, so are running the same config as normal. I
initially replaced the run target with simply:

$(ENV)/bin/gunicorn  --config /srv/gunicorn/sso_wsgi.py

For reference, that /srv/gunicorn/sso_wsgi.py looks like so:
https://pastebin.canonical.com/144274/

We override a few bits with cli args (like logs and timeouts) in make
run, like we did previously. But it makes the gunicorn run by 'make
run' to be as close as possible to the charm gunicorn, by reusing the
config. This is all wrapped up in a helper/debugging script that the
charm creates (/srv/gunicorn/run_sso_wsgi.sh), which runs gunicorn in
the foreground, rather than via upstart. Very useful for spotting
errors on the units, and I just reused it here. It decouples file
paths a bit more.

2) When we do make run, we want to use the same port (8080). So, the
helper script stops the upstart gunicorn first. In order to start it
up again when we're done, is uses bash signal traps. This could have
been done in a long one-liner in the makefile perhaps, but I felt it
was bet done in a separate script.

FWIW: that script is just:

#!/bin/bash
sudo service gunicorn stop
trap 'sudo service gunicorn start' INT
bash /srv/gunicorn/run_sso_wsgi.sh $@

We could actually get rid of this, perhaps. Some thing like:

run: gunicorn-stop
    @trap 'sudo service gunicorn start' INT;
/srv/gunicorn/run_sso_wsgi.sh --bind=$(ARGS) --reload
--error-logfile=- --access-logfile=- --timeout=99999

would probably work. I can have a look at that if you've prefer to get
rid of the script?


>> +     -./scripts/run-gunicorn.sh --bind=$(ARGS) --reload --error-logfile=- --access-logfile=- --timeout=99999
>> +
>> +## run acceptance tests in development
>> +acceptance-dev:
>> +     ./scripts/acceptance-dev-juju.sh $(ARGS)
>> +.PHONY: acceptance
>> +
>> +## Setup db with test data for acceptance tests
>> +acceptance-db: db-reset
>> +     $(DJANGO_MANAGE) loaddata test
>> +     $(DJANGO_MANAGE) create_test_team
>> +     # add openid RP config
>> +     # we need this to explicitly allow unverified logins
>> +     $(DJANGO_MANAGE) add_openid_rp_config $$SST_BASE_URL/consumer --allow-unverified --allowed-user-attribs=\"fullname,nickname,email,language,account_verified\"
>> +
>>
>> === modified file 'src/identityprovider/tests/test_command_cleanup.py'
>> --- src/identityprovider/tests/test_command_cleanup.py        2015-04-30 17:20:09 +0000
>> +++ src/identityprovider/tests/test_command_cleanup.py        2015-11-17 13:23:47 +0000
>> @@ -48,8 +48,8 @@
>>
>>      def make_test_accounts(self, count=0, date_created=None):
>>          for i in xrange(count):
>> -            email = self.factory.make_email_address(prefix='isdtest+',
>> -                                                    domain='canonical.com')
>> +            email = self.factory.make_email_address(prefix='testemail+',
>> +                                                    domain='example.com')
>
> What's the rationale for this change? The job needs to clear from the DB all email addressed created by the acceptance tests, which use the isdtest+ prefix, so changing it will not trigger the right cleanup.
>
>>
>>              account = self.factory.make_account(email=email)
>>              if date_created is not None:
>>
>> === modified file 'src/webui/tests/test_views_ui.py'
>> --- src/webui/tests/test_views_ui.py  2015-11-10 19:47:30 +0000
>> +++ src/webui/tests/test_views_ui.py  2015-11-17 13:23:47 +0000
>> @@ -606,7 +606,6 @@
>>
>>      def assert_logged_out_in_sso(self, response):
>>          self.assertTemplateUsed(response, 'registration/logout.html')
>> -        self.assertContains(response, 'You have been logged out')
>
> Would you remind me why we were removing this?

This was the brittle test. If you had run makemessages locally, then
the page text would be german, and thus this language check would
fail. james_w feit that assert the text was not adding anything, since
we were asserting for 200 and that the correct template was used
already, so we cut it. Ideally should have been separate MP, but it
got slipped in with this one.

>>          self.assertIsInstance(response.context['user'], AnonymousUser)
>>
>>      def assert_redirect_as_per_request(self, response, redirected_to):
>
>
> --
> https://code.launchpad.net/~bloodearnest/canonical-identity-provider/sso-dev/+merge/277474
> You are the owner of lp:~bloodearnest/canonical-identity-provider/sso-dev.