Rename VMs for clarity #407
Conversation
… sd-viewer See #285 for background and discussion
|
@emkll I was able to verify that this PR works as expected, following the testing instructions in the PR body. I'm leaving it in draft for now, but it's ready for you to take as spin :) |
…ion-viewer Optional follow-up to freedomofpress/securedrop-workstation#407
|
In a separate branch I've created here and in the packaging repo, I've pushed optional follow-up changes to rename the freedomofpress/securedrop-builder@05b488b I've successfully built a |
There was a problem hiding this comment.
This is great work, I have not encountered any issues based on my local testing:
Followed testing plan as described in pr description:
- make clean && make all on this branch (make sure you have no conflicting sd-app VM in place)
- Verify that all VMs are named as described.
- Build a new securedrop-client package on the vm-rename branch of securedrop-client, and install it in the sd-app-buster-template. Restart sd-app to pick up the change.
- Run the client. Verify that you can successfully log in, download, open, and export files
Performed further testing as folllows
-
make testall tests pass -
make {sd-viewer, sd-app, sd-devices}andmake remove-{sd-viewer, sd-app, sd-devices}work as expected
I think the order of operations should be as follows:
Review and merge at the same time:
0. Finish review (but not merge) this PR
- https://github.com/freedomofpress/securedrop-client/pull/701/files
- VM rename per https://github.com/freedomofpress/securedrop-workstation/issues/285 securedrop-builder#128
Once daily builds cron job will run, existing client nightlies will break on any workstation not provisioned on this branch.
- Merge this PR.
Inform all users to run make clean, make clone, make all on latest master of this repo.
The two others are test/docs only, and can be merged at any point.
Regarding renaming the sd-svs-disp package to sd-viewer, I would advocate we do it separately from this PR as it will not break existing installs, but will simply not update the sd-svs-disp package, (until someone runs make sd-viewer on the updated branch)
tests/test_dom0_config.py
Outdated
| "sd-svs-template", | ||
| "sd-svs-disp-template", | ||
| "sd-export-template", | ||
| "sd-app-template", |
There was a problem hiding this comment.
Instead of switching the names of historical templates, i would perhaps advocate we either
- keep them in place
- replace these with the legacy name (e.g. sd-svs-buster-template)
There was a problem hiding this comment.
how about 3. both? If the intent here is to ensure that old templates don't exist, we can just expand the list and call it OLD_TEMPLATES or something similar. because if we just do 2., the test won't fail for stretch users.
There was a problem hiding this comment.
yes I think old templates that contains STRETCH_TEMPLATES (prior to these changes) as well as the newly replaced templates (sd-svs-disp-buster-template, sd-svs-buster-template and sd-export-template) is a good idea
There was a problem hiding this comment.
Done in 550065c (went with DEPRECATED_TEMPLATES)
tests/test_viewer.py
Outdated
| @@ -5,7 +5,7 @@ | |||
|
|
|||
| class SD_SVS_Disp_Tests(SD_VM_Local_Test): | |||
There was a problem hiding this comment.
This class name was not changed. I see that it will be changed once we change the name of the package to sd-viewer, but in this case the class of the test represents the vm name and not the package installed
There was a problem hiding this comment.
That makes sense, should be fixed in ecd8b37
| @@ -12,38 +12,38 @@ TASK=${1:-default} | |||
| # 2. The AppVM must not be a DispVM template that used as the default DispVM | |||
| # for an AppVM, nor the system default DispVM. | |||
| if [[ $TASK == "prepare" ]]; then | |||
There was a problem hiding this comment.
Logic in this file in un-testable, but the diff looks good to me, I am wondering if, at this point, if it's worth keeping only for its historical value
|
I've run another test this morning based on the changes requested in freedomofpress/securedrop-client#701 (review). I have tested export (usb+print), reply, opening submissions), and all works as expected on that client version with the name change. One outstanding item that wasn't addressed in this PR was updated the DataFlow Diagram (https://github.com/freedomofpress/securedrop-workstation/blob/vm-rename-preferred-variant/docs/images/data-flow-diagram.png). I will be performing the following tasks:
|
There was a problem hiding this comment.
Thanks @eloquence, changes look good to me, based on testing/release checklist in #407 (comment). Every workstation will now need to make clean, make clone and make all to have these changes applied and be able to run SecureDrop Client 0.0.12+. Else they should pin 0.0.11 (not recommended). I have opened #408 to track the DFD update.
…ion-viewer Optional follow-up to freedomofpress/securedrop-workstation#407
Renames the following:
sd-svs->sd-appsd-export-usb->sd-devicessd-svs-disp->sd-viewerSee #285 for background and discussion.
Resolves #285. (Not changing
busternaming convention for cost/benefit reasons.)Dependent PRs:
Status
Ready for review
Deployment
Once all PRs are merged,
make clean && make allshould do the trick, but the client will only work if you are using a dev build or a nightly that incorporates the changes.Testing
make clean && make allon this branch (make sure you have no conflictingsd-appVM in place)securedrop-clientpackage on thevm-renamebranch ofsecuredrop-client, and install it in thesd-app-buster-template. Restartsd-appto pick up the change.