Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade all packages using pur #4318

Merged
merged 10 commits into from Sep 5, 2018
Merged

Upgrade all packages using pur #4318

merged 10 commits into from Sep 5, 2018

Conversation

@humitos
Copy link
Member

@humitos humitos commented Jul 2, 2018

As usual, upgrade all python packages from all requirements.

Just to know where we are regarding package versions and consider upgrade them if all test and manual QA pass.

for reqs in `ls requirements`;
  do pur --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth --requirement requirements/$reqs; 
done
django-mailgun==0.2.2
psycopg2==2.7.5
gunicorn==19.8.1
pysolr==3.7.0
Copy link
Member Author

@humitos humitos Jul 2, 2018

I think we never upgraded the deploy.txt file. So, we should be careful with them.

@@ -4,13 +4,13 @@ django-dynamic-fixture==2.0.0

# 3.6.1 and >3.2.5 is incompatible
# with pytest-describe 0.11.0
pytest==3.2.5
Copy link
Member Author

@humitos humitos Jul 2, 2018

If test passes, remove this docstring. Otherwise, do not upgrade pytest.

apipkg==1.4
execnet==1.5.0
Mercurial==4.4.2
Mercurial==4.6.1
Copy link
Member Author

@humitos humitos Jul 2, 2018

This is terrible :/

Mercurial 4.3 and newer require Python 2.7. Mercurial versions up to 4.2 support Python versions 2.6 to 2.7, provided they include the following standard library components:

https://www.mercurial-scm.org/wiki/SupportedPythonVersions

Copy link
Contributor

@agjohnson agjohnson Jul 9, 2018

I'm confused how we're even installing hg in our test suite if 3.x isn't supported. It seems this shouldn't work?

Copy link
Member Author

@humitos humitos Jul 10, 2018

I think we weren't hitting any of the Py 3.x problems but it wasn't supported. Although, it was possible to be installed and used:

Mercurial is actively being ported to Python 3. As of Mercurial 4.3, some commands work on Python 3. However, Python 3 is not yet a supported platform.

(from the bottom of the page I linked)

@agjohnson agjohnson added this to the Cleanup milestone Jul 9, 2018

# djangorestframework 3.7.x drops support for django 1.9.x
djangorestframework==3.6.4

# Filtering for the REST API
django-filter==1.1.0
django-filter==2.0.0
Copy link
Member Author

@humitos humitos Jul 23, 2018

Copy link
Contributor

@davidfischer davidfischer Aug 30, 2018

We don't use this for much so it should be fairly easy to test whether this works.

Copy link
Contributor

@davidfischer davidfischer Aug 30, 2018

I don't believe we can upgrade this one. Firstly, django-filter==2.0.0 requires Django 1.11 which we haven't upgraded to yet. Once we do that upgrade though, this migration should be fairly straight-forward. It looks like they changed the name of filter_fields => filterset_fields. We use that in the API

Copy link
Member Author

@humitos humitos Aug 30, 2018

I made a mistake when pushing my changes and I lost part of my work. There should be a note here saying what you just mentioned.

I'm sorry about that. I will re-add it.

for reqs in `ls requirements`;
do
  pur
  --skip django-tastypie,django,docker,elasticsearch,pyelasticsearch,commonmark,stripe,djangorestframework,mkdocs,django-allauth,django-filter,mercurial
  --requirement requirements/$reqs;
done
@humitos humitos force-pushed the humitos/pur/upgrade branch from ed3c02a to 0045f06 Aug 30, 2018
@humitos humitos force-pushed the humitos/pur/upgrade branch from 8c6eb6b to 3b1305a Aug 30, 2018
humitos added 3 commits Aug 30, 2018
Some block needed to be re-idented to fulfill this check.

Basically, if you have a "if something: return another" the "else"
clause is not needed since it will be the common case for the flow to
continue if the condition is not met.
@humitos humitos requested a review from Aug 30, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Aug 30, 2018

This PR is ready for review after some cleanup and fixes.

@@ -8,7 +8,7 @@
# This application object is used by any WSGI server configured to use this
# file. This includes Django's development server, if the WSGI_APPLICATION
# setting points here.
from django.core.wsgi import get_wsgi_application # pylint: disable=wrong-import-position
from django.core.wsgi import get_wsgi_application # pylint: disable=wrong-import-position # noqa
Copy link
Member

@stsewd stsewd Aug 30, 2018

I think we can leave only the # noqa comment, right?

@@ -47,6 +51,9 @@ dnspython==1.15.0

# VCS
httplib2==0.11.3

# GitPython 2.1.11 makes TestGitBackend.test_git_tags to fail because
# of an UnicodeError
Copy link
Member

@stsewd stsewd Aug 30, 2018

Maybe mention that we can update when we drop py2 support

Copy link
Member Author

@humitos humitos Aug 30, 2018

I'm starting to feel that this package is too unstable to rely all our building process on it. We just updated a bugfix version and it breaks our flow.

In the requirements, it says that it supports 2.7 or newer: https://gitpython.readthedocs.io/en/stable/intro.html#requirements

This is the commit that we think that introduced the issue: gitpython-developers/GitPython@7f08b77

Copy link
Member Author

@humitos humitos Aug 30, 2018

More examples:

I suppose that we should start thinking about another solution, unfortunately.

Copy link
Member Author

@humitos humitos Aug 30, 2018

Copy link
Member Author

@humitos humitos Aug 30, 2018

Tests in master and 2.1.11 fail. Also in 2.1.10... 😥

Copy link
Contributor

@agjohnson agjohnson Sep 4, 2018

Regarding dropping py27 support, this will be our v3.0 -- it might make sense to time 3.0 for all of our deprecations in jan 2019. i opened #4594 to discuss more

@humitos
Copy link
Member Author

@humitos humitos commented Sep 4, 2018

This is ready for re-review and merge in my opinion.

Besides upgrading the packages, since some linting packages were updated, I fixed some new lint issues that appeared here (thanks to @stsewd!)

I'd like to merge this soon to avoid merge conflicts.

Copy link
Contributor

@agjohnson agjohnson left a comment

LGTM! This is another that we'll want to simmer a little. We'll want to be careful with some of this releasing to prod, and probably warrants some QA.

@@ -8,7 +8,7 @@
# This application object is used by any WSGI server configured to use this
# file. This includes Django's development server, if the WSGI_APPLICATION
# setting points here.
from django.core.wsgi import get_wsgi_application # pylint: disable=wrong-import-position
from django.core.wsgi import get_wsgi_application # noqa
Copy link
Contributor

@agjohnson agjohnson Sep 5, 2018

I do generally prefer explicit disables, though I know that prospector was the one making this easier. For example, prospetor knows that pylint and flake8 (or whatever) throw an error on the same line, but pylint is ignoring -- it will therefore ignore the flake8 error as well. I don't think our pre-commit pass has anything comparable unfortunately.

#noqa becomes a generic exception handler of sorts, where who knows what else we're ignoring when we use #noqa

Copy link
Contributor

@agjohnson agjohnson Sep 5, 2018

Not sure if this is possible. worth making an issue on common?

Copy link
Member Author

@humitos humitos Sep 5, 2018

I also like being explicit here. I didn't find how to avoid the checking that was failing, though. It was something related to the import not at the top of the file and it wasn't pylint (I think it was flake --don't remember). That's why I just added # noqa.

@agjohnson agjohnson merged commit 8b7c279 into master Sep 5, 2018
1 check passed
@agjohnson agjohnson deleted the humitos/pur/upgrade branch Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants