alphagov / notifications-manuals

Documentations for Notify

Team manual

Our team manual lives in the Github wiki for this repository - https://github.com/alphagov/notifications-manuals/wiki

Any team member has the ability to edit it and is expected to when they see something not up to date or something that could be improved.

notifications-manuals's Issues

Running tests / flask / celery is inconsistent between repos

Problem

Each repo has a different set of often overlapping ways to run its code, only some of which are documented in its README.

• notifications-api:
  • ./scripts/run_{app,tests,celery}.sh
  • make test
  • make test-with-docker (not in README)
• notifications-admin:
  • ./scripts/run_{app,tests}.sh (tests not in README)
  • make test (not in README)
  • make test-with-docker (not in README)
• notifications-template-preview:
  • ./scripts/run{app,tests,celery}.sh (not in README)
  • make {_test,_run,_run-celery} (not in README)
  • make {test,run,run-celery}-with-docker
• document-download-api:
  • (no ./scripts/*)
  • make {run,test}
  • make {run,test}-with-docker (not in README)
• document-download-frontend:
  • ./scripts/run_{app,tests}.sh (not in README)
  • make {run,test} (test not in README)
  • make {run,test}-with-docker (not in README)
• notifications-antivirus:
  • ./scripts/run_{app,tests,celery}.sh (not in README)
  • make {_run,_run_app,_test} (not in README)
  • make {run,run-app,test}-with-docker (test not in README)
• notifications-python-client:
  • ./scripts/run_{tests,integration_tests}.sh (not in README)
  • make {test,integration-test} (not in README)
  • make {test,integration-test}-with-docker (not in README)
• notifications-utils:
  • ./scripts/run_tests.sh
  • make test (not in README)
• notifications-ftp:
  • ./scripts/run_{celery,tests}.sh
  • make test
  • make test-with-docker (not in README)
• notifications-functional-tests:
  • ./scripts/run*.sh
  • make test* (not in README)
  • make test*-with-docker (not in README)

Having multiple ways to do the same thing is confusing for new starters, and an unnecessary maintenance overhead. Some observations about the way these are used in practice:

• Asking the team indicates different people use each of these methods e.g. @servingUpAces and @CrystalPea use ./scripts, @leohemsted uses manual commands, @idavidmcdonald uses make.

• CI only uses ./scripts/run_tests.sh, except for document-download-api (see here) and all API clients (CI uses make test). In addition, CI uses make for all other things (like build and cf-deploy).

• The {run,test}-with-docker commands aren't used/necessary, except for notifications-template-preview, notifications-antivirus and client repos, where the dependencies are hard to install locally.

Aside from running code, there is a similarly spectrum of how to setup each app: ./scripts/bootstrap.sh, make build, and manual commands, all of which differ depending on the repo, and in CI.

Proposal

We should change all READMEs to refer to a single, standard way of running code (tests / flask / celery). This should be make because it's a standard automation tool, and because it supports a variety of other tasks.

• At the same time, we should try and standardise the naming of tasks, where these are currently inconsistent: test (run all linting and tests), build (bootstrap), run-flask, run-celery.

We should remove *-with-docker tasks from all Makefiles unless they are necessary due to dependency issues. This is because using Docker is slower and more abstract than running native code.

We should switch CI to use make test instead of ./scripts/run_tests.sh (or manual commands). This is because the way we build a project locally should match the way CI does it, to avoid unnecessary surprises.

• We should check CI can reliably run make and still fails a build appropriately (based on the exit code).

[Stretch] We should investigate moving ./scripts/* into the Makefile where practical (example of practical), so that these are more clearly defined as providing some kind of complex functionality.

[Stretch] We should investigate moving ./scripts/bootstrap.sh into the Makefile, as a standard make bootstrap task. This is for consistency with the way we (now) recommend running code.

Roadmap

This will take some time to implement, as it affects all repos and there will doubtless be subtleties not captured here. We should aim to apply the change incrementally. I'd suggest dividing the work into stages:

1. Do as much as we can without affecting CI. This includes: updating the repo README, removing redundant *-with-docker tasks, moving some ./scripts/* into Makefile (except ./scripts/run_tests.sh`, which will still be used by CI).

2. Update CI to use the Makefile for running tests, and potentially bootstrapping.

1. Do as much as we can without affecting CI

• notifications-api
• notifications-admin
• notifications-template-preview
• document-download-api
• document-download-frontend
• notifications-antivirus:
• notifications-*-client
• notifications-utils
• notifications-ftp
• notifications-functional-tests
• notifications-paas-autoscaler
• notifications-tech-docs

2 Update CI to use the Makefile for running tests

• Update Concourse pipelines for apps to use make test
• Update Concourse pipelines for functional tests to use make
• Move ./scripts/run_tests.sh into Makefiles

3 Ideas for follow-up issues

• Deploying apps is inconsistent between Makefiles and Concourse
• Freezing requirements is inconsistent between apps