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

Make application code Python 3 compliant #4239

Merged
merged 47 commits into from Apr 16, 2019
Merged

Make application code Python 3 compliant #4239

merged 47 commits into from Apr 16, 2019

Conversation

kushaldas
Copy link
Contributor

@kushaldas kushaldas commented Mar 6, 2019

Status

Ready for review

Description of Changes

Moving the application code base into Python3 in Xenial.

Testing

  1. There should be a new Python 3 test CI job that is passing, all other CI jobs should pass.

  2. Tests should pass on Python 2 and Python 3

BASE_OS=xenial PYTHON_VERSION=3 make test
BASE_OS=xenial PYTHON_VERSION=2 make test
  1. Dev envs should work on both Python 2 and Python 3:
PYTHON_VERSION=3 BASE_OS=xenial make dev
PYTHON_VERSION=2 BASE_OS=xenial make dev

Deployment

Until we are ready to deploy Python 3 in a virtualenv, we'll continue to deploy using Python 2 on production servers. However, all changes once this PR are merged should work under both Python 2 and Python 3.

Checklist

If you made changes to the server application code:

  • Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

If you made non-trivial code changes:

  • I have written a test plan and validated it for this PR

@eloquence
Copy link
Member

Hey @kushaldas, for draft WIP PRs like that, could you use GitHub's draft pull request feature? That will make it easier to distinguish WIP PRs at a glance, and you can in fact avoid the scary all caps headlines (since a draft PR cannot be merged until it is promoted).

@redshiftzero
Copy link
Contributor

Hey @kushaldas do you agree that the plan is:

  1. Ensure that the SecureDrop application code continues to work on Python 2 in production.
  2. Make changes to the SecureDrop application code such that it also works on Python 3, and adding a CI job to ensure this is the case going forward?

@redshiftzero redshiftzero changed the title [WIP] DO NOT MERGE [WIP] Make application code Python 3 compliant Mar 7, 2019
@redshiftzero redshiftzero changed the title [WIP] Make application code Python 3 compliant Make application code Python 3 compliant Apr 12, 2019
@codecov-io
Copy link

Codecov Report

Merging #4239 into develop will decrease coverage by 0.28%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4239      +/-   ##
===========================================
- Coverage     84.7%   84.41%   -0.29%     
===========================================
  Files           43       43              
  Lines         2785     2798      +13     
  Branches       304      307       +3     
===========================================
+ Hits          2359     2362       +3     
- Misses         358      363       +5     
- Partials        68       73       +5
Impacted Files Coverage Δ
securedrop/securedrop/secure_tempfile.py 91.52% <0%> (-8.48%) ⬇️
securedrop/securedrop/crypto_util.py 93.22% <0%> (-3.28%) ⬇️
securedrop/securedrop/source_app/main.py 92.56% <0%> (-1.23%) ⬇️
securedrop/securedrop/manage.py 78.41% <0%> (-0.35%) ⬇️
securedrop/securedrop/source_app/info.py 100% <0%> (ø) ⬆️
securedrop/securedrop/i18n_tool.py 94.17% <0%> (+0.03%) ⬆️
securedrop/securedrop/journalist_app/utils.py 87.57% <0%> (+0.07%) ⬆️
securedrop/securedrop/models.py 93.39% <0%> (+0.58%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ed6b88...c8e7ec9. Read the comment docs.

Copy link
Contributor

@heartsucker heartsucker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nits, but nothing that looks terribly wrong. I think some of these nits need to be cleaned up so we won't end up with extra parentheses in the codebase forever, but some changes matter less because they'll go away when we drop python2 support.

Also, there are a bunch of places where 2to3 added list(...) around places where we made iterators out of dicts (keys, items, values). It looks like most of these aren't necessary.

.circleci/config.yml Outdated Show resolved Hide resolved
securedrop/dockerfiles/xenial/python2/Dockerfile Outdated Show resolved Hide resolved
securedrop/i18n_tool.py Outdated Show resolved Hide resolved
securedrop/journalist_app/api.py Show resolved Hide resolved
securedrop/manage.py Outdated Show resolved Hide resolved
securedrop/tests/conftest.py Show resolved Hide resolved
securedrop/tests/conftest.py Outdated Show resolved Hide resolved
securedrop/tests/test_journalist_api.py Outdated Show resolved Hide resolved
@eloquence eloquence added this to Ready for review in SecureDrop Team Board Apr 15, 2019
@redshiftzero redshiftzero moved this from Ready for review to Under Review in SecureDrop Team Board Apr 15, 2019
@eloquence
Copy link
Member

eloquence commented Apr 15, 2019

I would suggest that this PR add a section to https://docs.securedrop.org/en/release-0.12.1/development/setup_development.html specifying what you're stating in the PR summary: how to build the dev environment for Python 3, and how to run the tests for Python 3, at least while both Python 2 and Python 3 are supported in development.

If this is done, this PR can resolve #997 (which has been re-scoped to describe stage 1 of the Python 3 migration).

@heartsucker heartsucker merged commit 4410a6c into develop Apr 16, 2019
SecureDrop Team Board automation moved this from Under Review to Done Apr 16, 2019
@heartsucker heartsucker deleted the pyyy3 branch April 16, 2019 10:43
@heartsucker
Copy link
Contributor

There were still some place with double parentheses for print and list(some_dict.keys()) and such, but those are relatively minor and can be cleaned up later. I think it was more important to get this merged in than to nitpick it to death.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants