-
Notifications
You must be signed in to change notification settings - Fork 693
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
Adds dev-focal to run SecureDrop on Focal container #5544
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes @kushaldas , some preliminary comments inline. There are 33 tests failing here under Focal:
tests/test_alembic.py::test_alembic_head_matches_db_models FAILED [ 0%]
tests/test_alembic.py::test_alembic_migration_downgrade[523fff3f969c] FAILED [ 3%]
tests/test_alembic.py::test_alembic_migration_downgrade[60f41bb14d98] FAILED [ 4%]
tests/test_alembic.py::test_alembic_migration_downgrade[48a75abc0121] FAILED [ 4%]
tests/test_alembic.py::test_alembic_migration_downgrade[35513370ba0d] FAILED [ 4%]
tests/test_alembic.py::test_alembic_migration_downgrade[3da3fcab826a] FAILED [ 4%]
tests/test_alembic.py::test_alembic_migration_downgrade[b58139cfdc8c] FAILED [ 5%]
tests/test_alembic.py::test_alembic_migration_downgrade[f2833ac34bb6] FAILED [ 6%]
tests/test_alembic.py::test_alembic_migration_downgrade[a9fe328b053a] FAILED [ 6%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[fccf57ceef02] FAILED [ 6%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[3d91d6948753] FAILED [ 6%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[60f41bb14d98] FAILED [ 7%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[2d0ce3ee5bdc] FAILED [ 7%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[e0a525cbab83] FAILED [ 7%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[b58139cfdc8c] FAILED [ 8%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[f2833ac34bb6] FAILED [ 8%]
tests/test_alembic.py::test_downgrade_with_data[b58139cfdc8c] FAILED [ 13%]
tests/test_crypto_util.py::test_delete_reply_keypair FAILED [ 17%]
tests/test_crypto_util.py::test_delete_reply_keypair_pinentry_status_is_handled FAILED [ 17%]
tests/test_integration.py::test_submit_message FAILED [ 22%]
tests/test_integration.py::test_submit_file FAILED [ 22%]
tests/test_journalist.py::test_delete_source_deletes_source_key FAILED [ 41%]
tests/test_rm.py::test_secure_delete_capability FAILED [ 60%]
tests/test_source.py::test_metadata_route FAILED [ 68%]
tests/test_source.py::test_metadata_v2_url FAILED [ 68%]
tests/test_source.py::test_metadata_v3_url FAILED [ 68%]
tests/functional/test_source_metadata.py::TestInstanceMetadata::test_instance_metadata FAILED [ 79%]
tests/pageslayout/test_journalist.py::TestJournalistLayout::test_col_flagged[en_US] FAILED [ 86%]
tests/pageslayout/test_journalist.py::TestJournalistLayout::test_col_flagged[ar] FAILED [ 86%]
tests/pageslayout/test_journalist.py::TestJournalistLayout::test_flag[en_US] FAILED [ 91%]
tests/pageslayout/test_journalist.py::TestJournalistLayout::test_flag[ar] FAILED [ 91%]
tests/pageslayout/test_source.py::TestSourceLayout::test_source_flagged[en_US] FAILED [ 97%]
tests/pageslayout/test_source.py::TestSourceLayout::test_source_flagged[ar] FAILED [ 97%]
securedrop/bin/run-test
Outdated
@@ -37,6 +37,8 @@ mkdir -p "../test-results" | |||
export PAGE_LAYOUT_LOCALES | |||
export TOR_FORCE_NET_CONFIG=0 | |||
|
|||
python3 -m pip install pytest pytest-xdist pluggy -U |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you've identified in the PR description, it would definitely make sense to move this to develop-requirements
and/or test-requirements
pinning the proper version and hashes. Since this doesn't affect the app code dependencies, we don't need a diff review, and I'm guessing newer versions should work under Xenial
@@ -0,0 +1,74 @@ | |||
# ubuntu 16.04 image from 2019-03-12 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this comment needs to be updated?
@@ -0,0 +1,74 @@ | |||
# ubuntu 16.04 image from 2019-03-12 | |||
FROM ubuntu@sha256:2e70e9c81838224b5311970dbf7ed16802fbfe19e7a70b3cbfa3d7522aa285b4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this Dockerfile introduced here is pretty much identical to the Xenial Dockerfile, what do you think about using Docker args to feed the Ubuntu container hash and python version to the Dockerfile so that we only maintain a single dockerfile? This is the approach used in the Workstation dom0 build logic in https://github.com/freedomofpress/securedrop-workstation/pull/612/files
In the past we've used BASE_OS
variable and it may indeed be easier for us to continue going that route, especially since we aren't planning supporting both Focal and Xenial for very long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have different packages installed in this container. I would love to keep in this BASE_OS
style two different dockerfiles.
f41cdf8
to
602f593
Compare
Due to the updating of the |
Status update is in this commit message: 19f527b |
c4bf136
to
6f87968
Compare
Still requires update for |
pip gave |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make dev-focal
is failing due to TBB browser version not available https://github.com/freedomofpress/securedrop/pull/5544/files#diff-96bf85e6e44291b96a0917ee04d234a6986bca1858f16f195446ed697d4bf53eR26
https://github.com/freedomofpress/securedrop/pull/5544/files#r497806221 is a suggestion to minimize maintenance through a single docker file for both focal and xenial
I will update this.
I commented above at #5544 (comment) to point out that the Dockerfiles have different dependencies installed. And just maintaining two different versions makes the whole process much simpler. |
|
Sorry, just now updated the test instruction, we can not run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`make dev-focal` will start a Focal container with SecureDrop running. Also updates the gpg2 --import command to import into the pubring.gpg keyring file explictly. Related Ansible change is tracked via #5499
Getting an error where functional tests aren't running in
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make test-focal
is now working for me locally, ran out of memory in my VM (thanks @kushaldas for the assist). Only two tests are failing inside the Focal container, those are captured in #5592.
CI is also green, good to merge from my perspective
Status
Ready for review.
Towards #5524
Description of Changes
make dev-focal
will start a Focal container with SecureDroprunning.
Also updates the gpg2 --import command to import into the
pubring.gpg keyring file explictly. Related Ansible change
is tracked via
#5499
The tests in Focal will fail to run, and will be fixed via #5585
Testing
make dev-focal
to see if it starts a container on Focalmake dev
should start SecureDrop on xenialmake test
should not have any error.make test-focal
will show test failures (as we are slowly fixing them forFocal
)Deployment
Any special considerations for deployment? Consider both:
Checklist
If you made changes to the server application code:
make lint
) and tests (make test
) pass in the development containerIf you made changes to
securedrop-admin
:make -C admin test
) pass in the admin development containerIf you made changes to the system configuration:
If you made non-trivial code changes:
If you made changes to documentation:
make docs-lint
) passed locallyIf you added or updated a code dependency:
Choose one of the following: