Test pydata application #1073

aditnryn · 2016-11-04T15:59:01Z

PR created to test whether Travis creates multiple jobs.

aditnryn · 2016-11-05T14:39:44Z

.travis.yml

 python:
  - 3.5
+
+env:
+  - PYDATA=true


PyData app introduces a migration since it adds a choice argument to the sponsorship amount field. So, I do not check for missing migrations here.

~~Where is that migration? I can't find it.~~

OK, I see it.

aditnryn · 2016-11-05T14:46:31Z

@pbanaszkiewicz @chrismedrela
I have added another job on Travis that runs the test suite while the pydata app is deployed.
Please review.

pbanaszkiewicz · 2016-11-06T09:30:37Z

.travis.yml

+      git pull -q --no-edit origin develop;
+    fi;
+  - if [[ $PYDATA == true ]]; then
+      git pull -q --no-edit origin deploy-pydata;


What exactly is in deploy-pydata?

Are you sure git pull is the right way to go?

Can we use git config --local instead of git config --global?

What exactly is in deploy-pydata?

Adds pydata to INSTALLED_APPS and urlconfig. See the commit cf8603aa

Are you sure git pull is the right way to go?

Isn't pull an aggregate command that executes fetch and merge. Shouldn't be a problem.

Can we use git config --local instead of git config --global?

The instances destruct after every test. It does not make a difference.

Adds pydata to INSTALLED_APPS and urlconfig. See the commit cf8603a

Why not conditionally include it in the config? It could be controlled via envvar.

Isn't pull an aggregate command that executes fetch and merge. Shouldn't be a problem.

Hmm well I can see some issues and questions here.

You're assuming that someone wants to merge into develop, which is not always true.

You're merging develop into PR branch (this may be the same as merging PR branch into develop - I'm not sure).

What happens when you get merge conflict. The test suite doesn't run?

And finally, according to the travis-ci docs:

Rather than test the commits that have been pushed to the branch the pull request is from, we test the merge between the origin and the upstream branch. To only build on push events, you can disable Build on Pull Requests from your repository settings.

So… is this what @chrismedrela intended in #1066?

Why not conditionally include it in the config? It could be controlled via envvar.

Nice suggestion. I am starting to reconsider #938. By overriding the envvar DJANGO_SETTINGS_MODULE, we can switch between a normal deployment and a PyData deployment.

So… is this what @chrismedrela intended in #1066?

Please refer to the discussion at #1066. As you pointed out, Travis does merge the develop branch before testing.

I have removed the fix for #1066. Also, I am moving the discussion out of this comment thread.

aditnryn · 2016-11-07T10:25:24Z

@pbanaszkiewicz

Why not conditionally include it in the config? It could be controlled via envvar.

Nice suggestion. I am starting to reconsider #938. By overriding the envvar DJANGO_SETTINGS_MODULE, we can switch between a normal deployment and a PyData deployment.

chrismedrela · 2016-11-08T17:50:00Z

.travis.yml

+        exit 1;
+      fi;
+    fi;
+


@narayanaditya95 Could you create a new PR, exactly the same as this one, but please remove the last migration file (0122_auto_20161101_1441.py) to proof that Travis build fails when there are missing migrations?

chrismedrela · 2016-11-08T17:55:36Z

I'm 👍 for merging when these three issues above are addressed. I've created a new issue #1075 to address monkey patching issue.

aditnryn · 2016-11-08T22:12:57Z

@chrismedrela
Are you fine with the current implementation of merging a branch, or would you rather consider a different approach that I suggested to @pbanaszkiewicz

Nice suggestion. I am starting to reconsider #938. By overriding the envvar DJANGO_SETTINGS_MODULE, we can switch between a normal deployment and a PyData deployment.

chrismedrela · 2016-11-09T14:44:17Z

Nice suggestion. I am starting to reconsider #938. By overriding the envvar DJANGO_SETTINGS_MODULE, we can switch between a normal deployment and a PyData deployment.

@narayanaditya95 I'd like to have everything on one branch (develop). Maintaining second branch (deploy-pydata) would be a lot of work, because of backporting everything from develop to deploy-pydata. So, yes, I'm 👍 for varying between deployments with envvars, with one important modification: I'd like to have one settings.py file that reads from an ENABLE_PYDATA envvar and contains conditionals like if ENABLE_PYDATA:. IMO that is simpler than having two separate settings.py files and keeping them in sync.

chrismedrela · 2016-11-09T14:46:50Z

BTW this is exactly what @pbanaszkiewicz has proposed:

What exactly is in deploy-pydata?

Adds pydata to INSTALLED_APPS and urlconfig. See the commit cf8603a

Why not conditionally include it in the config? It could be controlled via envvar.

1. Add fixtures introduced by pydata app in all tests 2. Mark tests that should fail as expectedFailures 3. Change amount field to one accepted by pydata app

aditnryn · 2016-11-10T16:37:21Z

@chrismedrela Please review.

chrismedrela

This one is tough, but I think we're pretty close to get this merged. :-)

One general question: right now, we've only one Travis job. It runs tests with pydata enabled. Should not we have another job with pydata disabled?

chrismedrela · 2016-11-10T17:37:45Z

amy/settings.py

+    INSTALLED_APPS.insert(
+        INSTALLED_APPS.index('workshops'),
+        'pydata'
+    )


Could you replace it with:

INSTALLED_APPS = [ 'django.contrib.auth', 'django.contrib.contenttypes', 'django.contrib.sessions', 'django.contrib.messages', 'django.contrib.staticfiles', 'workshops', ] if ENABLE_PYDATA: INSTALLED_APPS += [ 'pydata', ] INSTALLED_APPS += [ # this should be after 'workshops' because templates in # 'templates/registration/' clash 'django.contrib.admin', 'crispy_forms', 'selectable', 'django_countries', 'django_filters', 'reversion', 'rest_framework', 'api', 'extforms', 'captcha', 'compressor', 'social.apps.django_app.default', 'debug_toolbar', ]

So that INSTALLED_APPS is defined in one place.

chrismedrela · 2016-11-10T17:38:17Z

amy/settings.py

@@ -338,6 +338,17 @@
    },
 }

+# settings for PyData application
+ENABLE_PYDATA = os.environ.get('AMY_ENABLE_PYDATA', False)


What are valid values for AMY_ENABLE_PYDATA envvar? Which ones are truthy and which ones are falsy? What if somebody sets AMY_ENABLE_PYDATA=false?

We should probably parse AMY_ENABLE_PYDATA the same way we parse AMY_DEBUG:

json.loads(os.environ.get('AMY_DEBUG', 'true'))

chrismedrela · 2016-11-10T17:41:20Z

.travis.yml

 script:
-  - python manage.py makemigrations --dry-run -e; export STATUS_CODE=$?; if [[ "$STATUS_CODE" == "0" ]]; then exit 1; fi
  - coverage run --source=amy,api,workshops manage.py test


@pbanaszkiewicz, BTW where does it sends the coverage data?

@narayanaditya95 should we include pydata in --source?

@pbanaszkiewicz, BTW where does it sends the coverage data?

The coverage command generates a report. The coveralls command sends it to Coveralls.

@narayanaditya95 should we include pydata in --source?

I am not sure what will happen if two Travis jobs send coverage data to Coveralls for the same commit. Will it break?
@pbanaszkiewicz

The coverage command generates a report. The coveralls command sends it to Coveralls.

Yes, and where is the report available? https://coveralls.io/github/swcarpentry/amy results in 404.

Only @pbanaszkiewicz has the permission to access coveralls.io

I don't think I ever set up coveralls, but the codebase was ready for it.

chrismedrela · 2016-11-10T17:44:18Z

amy/urls.py

+
+
+if settings.ENABLE_PYDATA:
+    urlpatterns.insert(1, url(r'^workshops/', include('pydata.urls')))


Could you change it in a similar way to INSTALLED_APPS?

chrismedrela · 2016-11-10T17:45:16Z

amy/settings.py

+        'pydata'
+    )
+    PYDATA_USERNAME_SECRET = os.environ.get('AMY_PYDATA_USERNAME')
+    PYDATA_PASSWORD_SECRET = os.environ.get('AMY_PYDATA_PASSWORD')


Are these two settings optional? That is, is None valid value for both username and password?

No. But we have checks in pydata/checks.py to ensure these variables are not None.

check_pydata_installed_before_workshops checks only if these variables are set, but doesn't check if they're not None.

chrismedrela · 2016-11-10T17:53:41Z

.travis.yml

+  - AMY_ENABLE_PYDATA=true
+    AMY_PYDATA_USERNAME=username
+    AMY_PYDATA_PASSWORD=password
+  - CHECK_MIGRATION=true


Is this necessary? We don't have two Travis jobs anymore.

Previously, you couldn't enable pydata and, at the same time, enable migrations checks. What did you changed, that it works now?

Yes, we need two Travis jobs. There are tests that are marked as expected failure by pydata app. We need to explicitly test them.

Even now pydata and migration aren't enabled at the same time. In the first Travis job, pydata is enabled. In the second, we check for migrations along with the standard deployment.

Oh, I see, these lines define two builds.

chrismedrela · 2016-11-10T19:00:24Z

@pbanaszkiewicz could you review the design?

aditnryn · 2016-11-10T20:46:38Z

amy/settings.py

@@ -33,6 +33,13 @@
    SECRET_KEY = None
 SECRET_KEY = os.environ.get('AMY_SECRET_KEY', SECRET_KEY)

+# settings for PyData application
+ENABLE_PYDATA = json.loads(os.environ.get('AMY_ENABLE_PYDATA', 'false'))


I would love to add a layer of abstraction over this expression. :)
It took me a while to realize why json parsing was used here.

Maybe just add a comment? :-)

aditnryn · 2016-11-10T20:56:43Z

@chrismedrela
Addressed all concerns.

chrismedrela · 2016-11-10T21:54:15Z

@narayanaditya95 thank you for your work on this PR! @pbanaszkiewicz please merge if you like it.

aditnryn force-pushed the test-pydata branch 21 times, most recently from 9deec81 to b062943 Compare November 5, 2016 14:36

aditnryn commented Nov 5, 2016

View reviewed changes

aditnryn force-pushed the test-pydata branch 2 times, most recently from 38d4cea to 28bc47b Compare November 5, 2016 17:43

aditnryn mentioned this pull request Nov 5, 2016

Configure Travis so that it tests merged changes #1066

Closed

pbanaszkiewicz reviewed Nov 6, 2016

View reviewed changes

aditnryn force-pushed the test-pydata branch from 28bc47b to 4e7ba84 Compare November 7, 2016 09:53

aditnryn added the PyData label Nov 7, 2016

chrismedrela reviewed Nov 8, 2016

View reviewed changes

chrismedrela mentioned this pull request Nov 8, 2016

Stop monkey patching models in pydata/apps.py #1075

Closed

aditnryn added 2 commits November 10, 2016 00:58

Allow tests to pass with pydata app installed.

75dbb4f

1. Add fixtures introduced by pydata app in all tests 2. Mark tests that should fail as expectedFailures 3. Change amount field to one accepted by pydata app

Add another job on Travis that tests the pydata app.

cd68e02

aditnryn force-pushed the test-pydata branch 2 times, most recently from 419a3e4 to c93da36 Compare November 10, 2016 16:28

chrismedrela suggested changes Nov 10, 2016

View reviewed changes

aditnryn added 2 commits November 11, 2016 02:04

Allow PyData app to be deployed using env variable

c4ca491

Adds check to raise errors when PYDATA variables are undefined.

d481129

aditnryn force-pushed the test-pydata branch from c93da36 to d481129 Compare November 10, 2016 20:39

aditnryn commented Nov 10, 2016

View reviewed changes

Replace "== None" with "is None"

49cd1b1

chrismedrela approved these changes Nov 10, 2016

View reviewed changes

pbanaszkiewicz approved these changes Nov 11, 2016

View reviewed changes

pbanaszkiewicz merged commit d92ad89 into carpentries:develop Nov 11, 2016

pbanaszkiewicz added component: deployment type: maintenance labels Nov 11, 2016

aditnryn deleted the test-pydata branch November 14, 2016 11:23

pbanaszkiewicz modified the milestones: v1.10, v1.9.1 Nov 21, 2016



		if settings.ENABLE_PYDATA:
		urlpatterns.insert(1, url(r'^workshops/', include('pydata.urls')))

Test pydata application #1073

Test pydata application #1073

Conversation

aditnryn commented Nov 4, 2016 • edited

Choose a reason for hiding this comment

chrismedrela Nov 8, 2016 • edited

Choose a reason for hiding this comment

aditnryn commented Nov 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditnryn commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismedrela commented Nov 8, 2016

aditnryn commented Nov 8, 2016

chrismedrela commented Nov 9, 2016

chrismedrela commented Nov 9, 2016

aditnryn commented Nov 10, 2016

chrismedrela left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditnryn Nov 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismedrela commented Nov 10, 2016

aditnryn Nov 10, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aditnryn commented Nov 10, 2016

chrismedrela commented Nov 10, 2016

aditnryn commented Nov 4, 2016 •

edited

chrismedrela Nov 8, 2016 •

edited

aditnryn Nov 10, 2016 •

edited

aditnryn Nov 10, 2016 •

edited