Implements "make clean" target #3532

conorsch · 2018-06-15T20:05:25Z

Status

Work in progress.

Description of Changes

Fixes #2395. Marking as WIP because I haven't yet confirmed it resolves all the cleanup we want. Special attention should be given to e.g. git clean -x for consideration.

Changes proposed in this pull request:

Testing

Try running this on your local dev repo, and see what gets removed! Warning: destructive action! rsync the repo elsewhere if you want rollback capability during review.

Deployment

Mostly affects the dev env. Technically admins could run make clean and break SSH access to servers—that would be bad. As it stands, they can still manually rm important files, so I think it's worth the cost to avoid potential problems like #3528.

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 changes to `securedrop-admin`:

Linting and tests (make -C admin test) pass in the admin development container

If you made changes to the system configuration:

Configuration tests pass

If you made non-trivial code changes:

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

If you made changes to documentation:

Doc linting (make docs-lint) passed locally

codecov-io · 2018-06-15T20:54:25Z

Codecov Report

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

@@             Coverage Diff             @@
##           develop    #3532      +/-   ##
===========================================
- Coverage    85.12%   85.04%   -0.09%     
===========================================
  Files           37       37              
  Lines         2367     2367              
  Branches       260      260              
===========================================
- Hits          2015     2013       -2     
- Misses         289      290       +1     
- Partials        63       64       +1

Impacted Files	Coverage Δ
models.py	`87.55% <0%> (-0.81%)`	⬇️
journalist_app/decorators.py	`100% <0%> (ø)`	⬆️
crypto_util.py	`96% <0%> (ø)`	⬆️
config.py	`85.36% <0%> (ø)`	⬆️

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 d3554e7...a426201. Read the comment docs.

redshiftzero · 2018-06-15T23:24:20Z

Makefile

@@ -147,6 +147,18 @@ self-signed-https-certs: ## Generates self-signed certs for TESTING the HTTPS co
 vagrant-package: ## Package up a vagrant box of the last stable SD release
 	@devops/scripts/vagrant_package.sh

+.PHONY: clean
+clean: ## DANGER! Purges all site-specific info and developer files from project.


what about adding a wee confirmation prompt (something like this) such that we minimize chance of someone accidentally running make clean not understanding what they are doing? Admittedly unlikely but since it's a simple change it's probably worthwhile

heartsucker · 2018-06-17T16:48:32Z

Would it not also make sense to make this reset a dev env by adding something like

if hash vagrant &> /dev/null; then
  vagrand destroy -f
fi

msheiny · 2018-06-22T14:14:20Z

i would also like to target blowing away docker containers here .. im happy to add it but ill also make another ticket if you think its out of scope. We'll have to add labels to all the containers first so we can better target them for removal

conorsch · 2018-06-26T01:08:56Z

Implemented the suggestions requested by @redshiftzero and @heartsucker. Example output:

$ make clean
WARNING: Proceeding will destroy all local customizations, including
application configuration, user credentials, Onion URLs, static assets,
VMs, and all git-ignored files. It is NOT possible to undo these changes!
To proceed, type 'yes': n  

Action declined, exiting...
 $ make clean
WARNING: Proceeding will destroy all local customizations, including
application configuration, user credentials, Onion URLs, static assets,
VMs, and all git-ignored files. It is NOT possible to undo these changes!
To proceed, type 'yes': yes

removed 'build/ossec-agent-2.8.2-amd64.deb'
removed 'build/ossec-server-2.8.2-amd64.deb'
removed 'build/securedrop-app-code-0.8.0~rc2-amd64.deb'
removed 'build/securedrop-config-0.1.1+0.8.0~rc2-amd64.deb'
removed 'build/securedrop-grsec-4.4.135-amd64.deb'
removed 'build/securedrop-keyring-0.1.1+0.8.0~rc2-amd64.deb'
removed 'build/securedrop-ossec-agent-2.8.2+0.8.0~rc2-amd64.deb'
removed 'build/securedrop-ossec-server-2.8.2+0.8.0~rc2-amd64.deb'
Removing vagrant VMs
Removing .vagrant/
Removing admin/.cache/
Removing admin/.coverage
Removing admin/.tox/
Removing admin/securedrop_admin.egg-info/
Removing admin/tests/__pycache__/
Removing docs/_build/
Removing molecule/builder/tests/__pycache__/

Only the string yes will be accepted as confirmation. Does not include @msheiny's docker cleanup actions, since as @msheiny notes, that'd require changes to the container build logic. (Please do open an issue to track, @msheiny!)

heartsucker

Minor nits, no blockers.

heartsucker · 2018-06-26T08:47:19Z

devops/clean

+    printf "\\n"
+    case "${user_confirmation}" in
+        yes ) remove_unwanted_files;;
+        * ) printf "Action declined, exiting...\\n"


I would vote for an exit 1 here.

Also extra backslash?

Motivated by https://github.com/koalaman/shellcheck/wiki/SC1117, but I agree it's confusing. Will revise.

Hmm I guess either way works then? Damn bash is so weird. I swear I will never fully understand it.

heartsucker · 2018-06-26T08:47:46Z

devops/clean

+# Cleans all build artifacts and custom config from the repo.
+# Intended for use on developer workstations. Should NOT be run
+# on production Admin Workstations.
+set -e


This set -e doesn't propagate to the remove_unwanted_files function which is probably where we want it?

Oh, but it does!

$ git diff diff --git a/devops/clean b/devops/clean index 6ccdbb8ff..99243c3f3 100755 --- a/devops/clean +++ b/devops/clean @@ -8,6 +8,11 @@ set -u function remove_unwanted_files() { + echo "BEFORE FALSE" + false + echo "AFTER FALSE" + exit 5 + # Remove any Onion URL info from previous instances. rm -vf install_files/ansible-base/app-source-ths \ install_files/ansible-base/app-ssh-aths \ $ make clean WARNING: Proceeding will destroy all local customizations, including application configuration, user credentials, Onion URLs, static assets, VMs, and all git-ignored files. It is NOT possible to undo these changes! To proceed, type 'yes': yes BEFORE FALSE Makefile:152: recipe for target 'clean' failed make: *** [clean] Error 1

The "AFTER FALSE" message never appears, due to set -e halting execution of the script.

Wow, I learned that wrong ages ago and just never relied on it in functions. Well never mind then :)

Resets the project to a pristine state. Explicitly removes dangling files from previous VM incarnations, such as "app-source-ths", "config.py", and "build/*.deb". Additionally runs `git clean -f -x`, which will purge all gitignored files from the directory. Anything prod-related, such as a pubkey for OSSEC email alerts, or the *site-specific vars file* will be removed. Admins should never run this command, since it will destroy site-specific info. The project documentation instructs Admins to use the `securedrop-admin` script, and the `Makefile` is only for developers. Therefore it seems safe to include a destructive action like this, given that it is not included in Admin-facing documentation. Closes #2395.

Allows us a bit more flexibility, e.g. prompting user for confirmation, to avoid mistakes. Sprinkled in liberal comments to make it obvious to future maintainers what's what.

More comprehensive "make clean" behavior, including removal of: * Vagrant VMs * Molecule VMs * temporary git repos cloned by Molecule (for upgrade testing) * git-clean even gitignored directories The output is fairly verbose, so it's obvious to the developer what's been changed in the environment.

conorsch · 2018-06-28T17:55:53Z

Revised, according to feedback from @redshiftzero and @heartsucker.

heartsucker · 2018-06-28T21:18:50Z

devops/clean

+    if hash vagrant &> /dev/null && [[ -d .vagrant/ ]]
+    then
+        printf "Removing vagrant VMs\\n"
+        vagrant destroy -f > /dev/null


So my initial recommendation was a little off. vagrant destroy -f returns non-zero if there are no live VMs (which is counter to what -f mean for most remove commands). This needs to be guarded with || echo 'No VMs removed.' perhaps.

@heartsucker The conditional block avoids the non-zero exit, since it checks for existence of the .vagrant/ dir. In general I'm a fan of verbose scripts, but the rest of the script prints deletions if-and-only-if they occurred, so I'm inclined to omit further changes here in the interest of consistency.

Hmm even with set -x, I get some failures.

$ vagrant up /staging/ --no-provision && make clean Bringing machine 'mon-staging' up with 'virtualbox' provider... Bringing machine 'app-staging' up with 'virtualbox' provider... ==> mon-staging: Importing base box 'bento/ubuntu-14.04'... ==> mon-staging: Matching MAC address for NAT networking... ==> mon-staging: Checking if box 'bento/ubuntu-14.04' is up to date... ==> mon-staging: A newer version of the box 'bento/ubuntu-14.04' is available! You currently ==> mon-staging: have version '201803.24.0'. The latest is version '201806.08.0'. Run ==> mon-staging: `vagrant box update` to update. ==> mon-staging: Setting the name of the VM: securedrop_mon-staging_1530607282211_90442 ==> mon-staging: Clearing any previously set network interfaces... ==> mon-staging: Preparing network interfaces based on configuration... mon-staging: Adapter 1: nat mon-staging: Adapter 2: hostonly ==> mon-staging: Forwarding ports... mon-staging: 22 (guest) => 2222 (host) (adapter 1) ==> mon-staging: Booting VM... ==> mon-staging: Waiting for machine to boot. This may take a few minutes... mon-staging: SSH address: 127.0.0.1:2222 mon-staging: SSH username: vagrant mon-staging: SSH auth method: private key ==> mon-staging: Machine booted and ready! ==> mon-staging: Checking for guest additions in VM... ==> mon-staging: Setting hostname... ==> mon-staging: Configuring and enabling network interfaces... ==> mon-staging: Machine not provisioned because `--no-provision` is specified. ==> app-staging: Importing base box 'bento/ubuntu-14.04'... ==> app-staging: Matching MAC address for NAT networking... ==> app-staging: Checking if box 'bento/ubuntu-14.04' is up to date... ==> app-staging: A newer version of the box 'bento/ubuntu-14.04' is available! You currently ==> app-staging: have version '201803.24.0'. The latest is version '201806.08.0'. Run ==> app-staging: `vagrant box update` to update. ==> app-staging: Setting the name of the VM: securedrop_app-staging_1530607313654_20237 ==> app-staging: Fixed port collision for 22 => 2222. Now on port 2200. ==> app-staging: Clearing any previously set network interfaces... ==> app-staging: Preparing network interfaces based on configuration... app-staging: Adapter 1: nat app-staging: Adapter 2: hostonly ==> app-staging: Forwarding ports... app-staging: 22 (guest) => 2200 (host) (adapter 1) ==> app-staging: Running 'pre-boot' VM customizations... ==> app-staging: Booting VM... ==> app-staging: Waiting for machine to boot. This may take a few minutes... app-staging: SSH address: 127.0.0.1:2200 app-staging: SSH username: vagrant app-staging: SSH auth method: private key ==> app-staging: Machine booted and ready! ==> app-staging: Checking for guest additions in VM... ==> app-staging: Setting hostname... ==> app-staging: Configuring and enabling network interfaces... ==> app-staging: Machine not provisioned because `--no-provision` is specified. + confirmation_prompt + cat WARNING: Proceeding will destroy all local customizations, including application configuration, user credentials, Onion URLs, static assets, VMs, and all git-ignored files. It is NOT possible to undo these changes! + read -r -p 'To proceed, type '\''yes'\'': ' user_confirmation To proceed, type 'yes': yes + case "${user_confirmation}" in + remove_unwanted_files + rm -vf install_files/ansible-base/app-source-ths install_files/ansible-base/app-ssh-aths install_files/ansible-base/app-journalist-aths install_files/ansible-base/mon-ssh-aths 'build/*.deb' + rm -rf molecule/upgrade/.molecule/sd-orig molecule/vagrant_packager/.molecule/sd-orig + find /home/heartsucker/code/freedomofpress/securedrop -type f -iname '*.pyc' -delete + find /home/heartsucker/code/freedomofpress/securedrop -type f -iname '*.retry' -delete + rm -rvf securedrop/static/.webassets-cache .venv + hash vagrant + [[ -d .vagrant/ ]] + printf 'Removing vagrant VMs\n' Removing vagrant VMs + vagrant destroy -f ==> app-prod: VM not created. Moving on... ==> mon-prod: VM not created. Moving on... ==> app-staging: Forcing shutdown of VM... ==> app-staging: Destroying VM and associated drives... ==> mon-staging: Forcing shutdown of VM... ==> mon-staging: Destroying VM and associated drives... Makefile:152: recipe for target 'clean' failed make: *** [clean] Error 2

@heartsucker I'm unable to reproduce your results:

$ vagrant up /staging/ --no-provision && make clean Bringing machine 'mon-staging' up with 'libvirt' provider... Bringing machine 'app-staging' up with 'libvirt' provider... ==> app-staging: Creating image (snapshot of base box volume). ==> mon-staging: Creating image (snapshot of base box volume). ==> app-staging: Creating domain with the following settings... ==> mon-staging: Creating domain with the following settings... [...snip...] WARNING: Proceeding will destroy all local customizations, including application configuration, user credentials, Onion URLs, static assets, VMs, and all git-ignored files. It is NOT possible to undo these changes! To proceed, type 'yes': yes Removing vagrant VMs Removing .vagrant/ $ vagrant --version Vagrant 1.9.1

Out of curiosity, which version of vagrant are you using? I don't mind adding the || you suggest! Just trying to figure out the cause, since if we have divergence, we should probably pin a version of vagrant to support (cf. #3350).

N.B. the vagrant changelog notes that in 2.0.1, the exit code for destroy changed:

commands/destroy: Exit 0 if vagrant destroy finds no running vms [GH-9251]

but that description doesn't match what you're seeing.

So on vagrant 2.0.1, I saw the same as @heartsucker:

$ make clean WARNING: Proceeding will destroy all local customizations, including application configuration, user credentials, Onion URLs, static assets, VMs, and all git-ignored files. It is NOT possible to undo these changes! To proceed, type 'yes': yes Removing vagrant VMs make: *** [clean] Error 2

Which confused me. So I upgraded to latest vagrant, which should be fine since the changelog indicates the exit codes were fixed after 2.0.2 (which apparently was the version that modified the exit code behavior).

Unfortunately, it turns out that even on latest, if I have a vagrant VM up and i make clean, it will still exit 2, due to the logic here, where vagrant destroy still returns 2 (see the bugfix they applied in 2.0.2 here: https://github.com/hashicorp/vagrant/pull/9251/files which doesn't fix this). I filed hashicorp/vagrant#10024 to figure out if this is a bug or not.

In the meantime, what about removing set -e?

Great debugging, @redshiftzero ! How frustrating.

In the meantime, what about removing set -e?

I'd vastly prefer @heartsucker's suggestion of || true simply on the vagrant destroy command. Acceptable?

Honestly, I don't really have any strong feelings about this as long as make clean does not produce an error when:

No VMs are deleted (@heartsucker's first case)

One VM is deleted (my case)

The guard proposed would address case one, but produce inaccurate output if the suggested || echo 'No VMs removed.' text is used, so as long as we change that text to be more general I'm cool with it

Try again, @redshiftzero ? I revised the logic to ignore errors, and made the messaging more general. For me, the following are true:

make clean with VMs displays "Removing VMs... done" with no error

make clean without VMs displays nothing, with no error.

Again, I'm on vagrant 1.9.1. If you see different results, happy to investigate further.

(Verified this behavior in both these cases 👍)

@redshiftzero

After some spelunking by @redshiftzero, turns out that Vagrant versions vary widely in the status codes reported by the destroy action. Let's simply ignore any errors returned rather than trying to inform the developer exactly what happened based on a specific status code and Vagrant version. The logic here should work equally well across all versions of Vagrant.

redshiftzero

LGTM

conorsch added the PR: Pending additional work label Jun 15, 2018

conorsch force-pushed the makefile-clean-target branch from 8f5445c to 227d949 Compare June 15, 2018 20:05

redshiftzero suggested changes Jun 15, 2018

View reviewed changes

conorsch requested a review from msheiny as a code owner June 26, 2018 01:07

heartsucker reviewed Jun 26, 2018

View reviewed changes

Conor Schaefer added 3 commits June 28, 2018 10:54

Converts Makefile "clean" target to script

3ceaddb

Allows us a bit more flexibility, e.g. prompting user for confirmation, to avoid mistakes. Sprinkled in liberal comments to make it obvious to future maintainers what's what.

conorsch force-pushed the makefile-clean-target branch from 7984b65 to 1c12363 Compare June 28, 2018 17:54

conorsch requested a review from heartsucker June 28, 2018 17:55

heartsucker reviewed Jun 28, 2018

View reviewed changes

redshiftzero approved these changes Jul 13, 2018

View reviewed changes

conorsch removed the PR: Pending additional work label Jul 13, 2018

redshiftzero merged commit b68e8c1 into develop Jul 13, 2018

redshiftzero deleted the makefile-clean-target branch July 13, 2018 23:48

conorsch mentioned this pull request Jul 26, 2018

Converts Vagrantfile to Molecule for staging #3644

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements "make clean" target #3532

Implements "make clean" target #3532

conorsch commented Jun 15, 2018

codecov-io commented Jun 15, 2018 •

edited

Loading

redshiftzero Jun 15, 2018

heartsucker commented Jun 17, 2018

msheiny commented Jun 22, 2018

conorsch commented Jun 26, 2018

heartsucker left a comment

heartsucker Jun 26, 2018

heartsucker Jun 26, 2018

conorsch Jun 28, 2018

heartsucker Jun 28, 2018

heartsucker Jun 26, 2018

conorsch Jun 28, 2018

heartsucker Jun 28, 2018

conorsch commented Jun 28, 2018

heartsucker Jun 28, 2018

conorsch Jul 2, 2018

heartsucker Jul 3, 2018

conorsch Jul 6, 2018

redshiftzero Jul 13, 2018

conorsch Jul 13, 2018

redshiftzero Jul 13, 2018

conorsch Jul 13, 2018

redshiftzero Jul 13, 2018

redshiftzero left a comment

Implements "make clean" target #3532

Implements "make clean" target #3532

Conversation

conorsch commented Jun 15, 2018

Status

Description of Changes

Testing

Deployment

Checklist

If you made changes to the server application code:

If you made changes to securedrop-admin:

If you made changes to the system configuration:

If you made non-trivial code changes:

If you made changes to documentation:

codecov-io commented Jun 15, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

heartsucker commented Jun 17, 2018

msheiny commented Jun 22, 2018

conorsch commented Jun 26, 2018

heartsucker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redshiftzero left a comment

Choose a reason for hiding this comment

If you made changes to `securedrop-admin`:

codecov-io commented Jun 15, 2018 •

edited

Loading