Add support for python 3 and drop python 2 support #48
Conversation
|
@morenol please review. |
Makefile
Outdated
| @@ -4,3 +4,13 @@ clean: | |||
|
|
|||
| test: | |||
| py.test | |||
| matrix: | ||
| include: | ||
| - python: 2.7 |
There was a problem hiding this comment.
If we are removing the python2.7 tests then I think that it is not needed the usage of six and the future imports
|
It looks good to me, but I would not remove the python2.7 tests until the internal job of jenkins that runs this is configured to use python3. So I think that we should keep the python2.7 tests and then after the jenkins job is changed we can drop python2.7 |
@jmbowman does the said Jenkins internal job still use python 2? and in that case, we shouldn't drop python 2 support here? |
|
Yes, it looks like both jobs need to be updated; they're using the
As far as I can tell, those are the only uses of this repository; once they're updated, it should be safe to drop the 2.7 support. They'll need to use 3.5 for now, since we don't have 3.8 on the Jenkins workers yet (I'll check today to see how close we are to getting that resolved). |
mraarif
force-pushed
the
py38-support
branch
2 times, most recently
from
0c1b9b5
to
273062a
Compare
|
So it seems like we won't be able to drop Python 2.7 support for a while due to details of how these jobs are currently run in Jenkins; there are plans to fix that, but it probably won't happen in the next few months. It's probably ok to merge any changes already made in this PR that add support for newer Python versions as long as the Python 2.7 support continues to work; but further work on upgrading this repo isn't a high priority since the production usage of it won't change for a while. |
mraarif
force-pushed
the
py38-support
branch
2 times, most recently
from
7f90a31
to
cfe7e88
Compare
|
As suggested I've only added support for Python 3.5 for now. |
mraarif
changed the title
There was a problem hiding this comment.
Given that I'm finding (admittedly minor) issues that could impact the execution of the Jenkins jobs, and DE doesn't actually plan to do the upgrade for a while, I'm starting to think we should just defer further work on this for now. It doesn't seem worth the risk of breaking the jobs and needing to spend time debugging it if we aren't going to actually do the Python upgrade now.
exporter/properties.py
Outdated
| @@ -28,6 +28,8 @@ | |||
| Can use wildcards. | |||
| """ | |||
|
|
|||
| from __future__ import absolute_import | |||
| from __future__ import print_function | |||
There was a problem hiding this comment.
These imports should be on the same line; ditto for the next file changed.
| @@ -1 +1,3 @@ | |||
| -c constraints.txt | |||
|
|
|||
| http://cdn.mysql.com/Downloads/Connector-Python/mysql-connector-python-1.2.2.zip | |||
There was a problem hiding this comment.
This file is still referenced in the Jenkins jobs by name, so we can't just rename it. We'd have to leave a stub in place that just includes a -r entry for the new file location.
mraarif
force-pushed
the
py38-support
branch
from
b199a64
to
5475db9
Compare
requirements/github.txt
Outdated
| # | ||
| # make upgrade | ||
| # | ||
| http://cdn.mysql.com/Downloads/Connector-Python/mysql-connector-python-1.2.2.zip # via -r requirements/github.in |
There was a problem hiding this comment.
Do we still need this file? I think it's just leftover from before we updated the Makefile to rename the file for compatibility with the existing Jenkins job code.
requirements/test.in
Outdated
| -c constraints.txt | ||
|
|
||
| -r base.txt | ||
| -r github.txt |
There was a problem hiding this comment.
Shouldn't this be updated to use the same filename generated in the Makefile?
There was a problem hiding this comment.
You mean it should be named as github_requirements.txt?
There was a problem hiding this comment.
@pwnage101 Do you want to control the timing on merging this, since it drops 2.7 support? I presume the Jenkins jobs also need to be updated to use Python 3.7 for their virtualenvs.
mraarif
force-pushed
the
py38-support
branch
from
1cf172e
to
a3d1cbc
Compare
|
@jmbowman could you point me to the Jenkins job that needs to switch its python version? or DE team would like to handle that update? |
mraarif
changed the title
|
I think Troy was planning to, but they're at https://github.com/edx/jenkins-job-dsl/blob/master/dataeng/jobs/analytics/AnalyticsEmailOptin.groovy and https://github.com/edx/jenkins-job-dsl/blob/master/dataeng/jobs/analytics/AnalyticsExporter.groovy if you want to try it. |
|
This was ultimately done via #73 |
Relevant JIRA issue here.