Skip to content

Fix configuration and Travis errors#175

Merged
knaperek merged 2 commits intoIdentityPython:masterfrom
OskarPersson:fix-conf-errors
Feb 5, 2020
Merged

Fix configuration and Travis errors#175
knaperek merged 2 commits intoIdentityPython:masterfrom
OskarPersson:fix-conf-errors

Conversation

@OskarPersson
Copy link
Contributor

Fixes regressions from #150 and #151

@peppelinux
Copy link
Member

@OskarPersson
Copy link
Contributor Author

Oops, completely missed that one. Still relevant for #150 though (right?)

@OskarPersson
Copy link
Contributor Author

Also @knaperek, I'm not sure why these PRs (including #166) was merged when Travis failed? I think you should configure this repo to

  1. report travis output on the PR pages and
  2. disallow merging of PRs where travis has failed

@OskarPersson
Copy link
Contributor Author

The Travis build still fails on Django 2.2+, though I can't reproduce the error locally.

@peppelinux
Copy link
Member

peppelinux commented Jan 17, 2020

Oops, completely missed that one. Still relevant for #150 though (right?)

Not Sure, selected_idps Will be Always false in that position, so the if Is useless I think.
Thank you for the feedback Oscar

@peppelinux
Copy link
Member

peppelinux commented Jan 17, 2020

The Travis build still fails on Django 2.2+, though I can't reproduce the error locally.

auth_group schema does not work there.
We should also remove support for py34 and py35.
Also only Django >= 2.2.9 should be supported cause of well known CVE.
I can do a pr on this but I'll take two weeks from now

@OskarPersson OskarPersson changed the title Fix config errors Fix configuration and Travis errors Jan 18, 2020
@OskarPersson
Copy link
Contributor Author

I've now updated the Travis dist to get rid of the schema errors and simultanesly dropped support for Python 3.4 (EOL 2019-03-18): I've kept Python 3.5 which has its EOL 2020-09-13:

https://devguide.python.org/devcycle/#end-of-life-branches
https://devguide.python.org/#status-of-python-branches

Travis build: https://travis-ci.org/knaperek/djangosaml2/builds/638821120

@knaperek This PR now contains a mixed bag of fixes, are you okay with merging it as-is or should I split it up into multiple PRs first?

@peppelinux
Copy link
Member

peppelinux commented Jan 18, 2020

Nice, if my point of view Means something here:

  • Ok on forceauthn and allowcreate, that works...
  • selected_idp should be refactored in the way I explained

Old versions of Django could be dropped as well

@peppelinux
Copy link
Member

this also should be refactored: https://github.com/knaperek/djangosaml2/pull/167/files

@OskarPersson
Copy link
Contributor Author

Regarding the change of selected_idp, if I remove the if-statement I get the following error from the tests:

FAIL: test_login_several_idps (djangosaml2.tests.SAML2Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/oskar/git/djangosaml2/.tox/py37-django22/lib/python3.7/site-packages/djangosaml2/tests/__init__.py", line 197, in test_login_several_idps
    self.assertEqual(url.hostname, 'idp2.example.com')
AssertionError: 'idp3.example.com' != 'idp2.example.com'
- idp3.example.com
?    ^
+ idp2.example.com
? 

@peppelinux
Copy link
Member

Regarding the change of selected_idp, if I remove the if-statement I get the following error from the tests:

FAIL: test_login_several_idps (djangosaml2.tests.SAML2Tests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/oskar/git/djangosaml2/.tox/py37-django22/lib/python3.7/site-packages/djangosaml2/tests/__init__.py", line 197, in test_login_several_idps
    self.assertEqual(url.hostname, 'idp2.example.com')
AssertionError: 'idp3.example.com' != 'idp2.example.com'
- idp3.example.com
?    ^
+ idp2.example.com
? 

Ok @OskarPersson That's important, my excuses here.

@peppelinux peppelinux mentioned this pull request Jan 18, 2020
@OskarPersson OskarPersson requested a review from knaperek January 18, 2020 18:21
@peppelinux peppelinux mentioned this pull request Jan 24, 2020
@OskarPersson
Copy link
Contributor Author

@knaperek Anything missing for merge?

@knaperek knaperek merged commit 257ba9a into IdentityPython:master Feb 5, 2020
@knaperek
Copy link
Collaborator

knaperek commented Feb 5, 2020

All good, tack så mycket!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants