educloudalliance / eca-auth-data

Common gateway of user data to all other services.
MIT License
0 stars 1 forks source link

Test refactor #15

Closed tkharju closed 8 years ago

tkharju commented 8 years ago

I am preparing this repo to work with travis-ci. In the process I am doing some refacoring, freezing of package versions, and possibly writing tests. I also updated django version but I am still going to make sure this project works with updated Django. This work is not ready yet.

tkharju commented 8 years ago

Remember before deployment that this change will require salt-changes on the server as location of settings and wsgi.py has changed.

tkharju commented 8 years ago

It seems that this change https://docs.djangoproject.com/en/1.9/releases/1.8/#abstractuser-last-login-allows-null-values makes if hard to support both 1.7 and 1.8 at the same time. I suggest we upgrade to to 1.8 and make a decision that older Django versions are not supported from this point on.

derega commented 8 years ago

I don't see how it is hard to support both 1.7 and 1.8 at the same time. We never do anything with user.last_login so it does not matter if it is nullable or not from our perspective. Field which we never use. In installs which use Django 1.7 it is not nullable and in installs which use Django 1.8 it is nullable. As this is coming from AbstractUser we never see this difference.

But still: We only support one specific Django version, and it is the one we are running in the ECA Beta environment. Nothing more, nothing less. We do not test all Django versions. We test only the one which we have picked. According to issue #14 this must be Django version 1.8, and only that one.

derega commented 8 years ago

Would it be possible to drop the suffixes from external data source module names?

For example:

from authdata.externaldata.dreamschool_source import DreamschoolDataSource

would be instead:

from authdata.externaldata.dreamschool import DreamschoolDataSource

or is there some reason for that suffix to be there?

Similar naming convention also for LDAP data sources. Instead of ldap_base.py and ldap_sources.py you would have ldap.py which has the base class, and then oulu.py.

The idea here would be to have only one external data source in each module. There could then be extensive documentation for each data source at the module level. Some sources also might have some utility functions, test harnesses, or something which is used only in that external data source. The separation would be clearer.

tkharju commented 8 years ago

Problem with supporting Django 1.7 and 1.8 at the same time is with the migrations. If we do the migrations in 1.7 then _user.lastlogin is no not nullable. If we do the migrations in 1.8 then the migration code is not backwards compatible with 1.7. We discussed this with @derega today and decided that it's better just to stick to 1.8.

In reference to renaming external sources I did this change in 7dc1b296c0d2aac8421b06f1e81ba1a65cd13b67. However I wouldn't rename ldap_base.py to ldap.py because we already have import ldap in the code.

Tests pass after this refactor and the PR could be merged. However deployment to server will require changes to salt pillar as the names and locations of the external sources have changed.

tkharju commented 8 years ago

This pull request is ready to be accepted. Closes #9 and closes #14