remove python 2 support in securedrop server code #4859
Conversation
recall that we have a separate dev env for the securedrop-admin CLI tool in admin/
redshiftzero
requested review from
conorsch,
emkll,
heartsucker,
kushaldas and
zenmonkeykstop
as code owners
|
Fall cleaning! This looks great. 🙂 Found a few lingering references in the docs (which I'd link to if GitHub didn't insist on rendering them 🙁 ):
And in |
There was a problem hiding this comment.
Thank you @redshiftzero ! Changes look good to me.
While grepping through the repo for references to python2, came across these two findings (I think it's fine to merge as-is, since the former would require a fix for running installs, perhaps a follow up, if necessary is more suitable):
-
Is this package strictly required for SecureDrop now? Since we are using a virtualenv, this package should no longer be necessary ?
It's unclear to me what this comment refers to, but simply flagging despite it looking like a comment for potential optimization.
I'll leave this around for a bit to give the opportunity for others to chime in, otherwise will merge this tomorrow morning
| @@ -146,9 +146,9 @@ safety: ## Run `safety check` to check python dependencies for vulnerabilities. | |||
| bandit: test-config ## Run bandit with medium level excluding test-related folders. | |||
| @command -v bandit || (echo "Please run 'pip install -U bandit'."; exit 1) | |||
| @echo "███ Running bandit..." | |||
| @bandit -ll --exclude ./admin/.tox,./admin/.venv,./admin/.eggs,./molecule,./testinfra,./securedrop/tests,./.tox,./.venv*,securedrop/config.py --recursive . | |||
| @bandit -ll --skip B322 --exclude ./admin/.tox,./admin/.venv,./admin/.eggs,./molecule,./testinfra,./securedrop/tests,./.tox,./.venv*,securedrop/config.py --recursive . | |||
There was a problem hiding this comment.
👍 an interested observer can see https://docs.openstack.org/bandit/latest/api/bandit.blacklists.html#b322-input for more details
There was a problem hiding this comment.
Note that we still have some Python2 code (securedrop-admin), but that will be moved to python3 before the next release, so I think it's fine to merge
That's a good catch. I think it should be removed. |
There was a problem hiding this comment.
Opened #4864 to follow-up on removal of libpython2.7-dev, this is good to merge, thanks @redshiftzero and @rmol for the review
Status
Ready for review
Description of Changes
Fixes #4699
Changes proposed in this pull request:
Testing
For this one you should be able to rely on diff review and CI (application unit tests):
app-testsjob (this is the python 3 application test job) should passsixcalls from app code and tests should be goneDeployment
as of 1.0.0 the paths I'm removing are no longer ran in production, the other changes are mostly dev/CI facing
Checklist
If you made changes to the server application code:
make lint) and tests (make -C securedrop test) pass in the development containerIf you made non-trivial code changes:
If you made changes to documentation:
make docs-lint) passed locally