Running tests / flask / celery is inconsistent between repos

[benthorner]

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

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

2 Update CI to use the Makefile for running tests

• [x] Update Concourse pipelines for apps to use make test
• [x] Update Concourse pipelines for functional tests to use make
• [x] 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

[quis]

When I worked on migrating the admin app to use piptools in the Christmas lull one of the things I wanted to do but didn’t (for the sake of keeping a small diff) was:

[tidy] up some of our bash scripts which would be better in the Makefile

So I’d support this proposal.

[leohemsted]

seems good and I'm in favour, only thing I have to note is that the -with-docker commands aren't just for antivirus/template-preview - they're also useful for all our client libraries so we dont need to install ruby, node, dot net, java etc to run integration or unit tests.

[idavidmcdonald]

Thanks Ben. I support your proposals.

Some things to look into when we do them:

• Does CI have make installed always?
• I want us to be sure that make will return correct exit statuses for us so the CI knows whether to fail the task or not

Seems like Leo, Chris and I generally support this so unless we hear any objections I think we can assume consensus. Feel free to do this stuff Ben, whether that be now or in 3 weeks time or just sporadically whenever you have a free 5 minutes. It doesn't just have to be you either, all devs should be making these little changes when they have a spare 5 minutes.

[klssmith]

Making things consistent sounds good to me.

In notifications-antivirus I always forget which of the commands (make run-app-with-docker and make run-with-docker) is for the Flask app and which is for the Celery app, so it would be useful to have a general pattern we follow.

[benthorner]

Thanks for the comments so far everyone! I'll try and briefly summarise where we are:

• @leohemsted good point about the clients. I've amended the description with that.

• @idavidmcdonald I agree we need to be careful when changing CI. I've amended the description to make a note of that.

• @klssmith I found that confusing as well - it's like the wrong way round! I've amended the description to suggest we standardise on the names we use for tasks at the same time we make other changes.

I've also appended a rough "roadmap" section to the description, based on @idavidmcdonald's comment - part of the reason for writing this up was realising it might take some time / thought.

[leohemsted]

agreed around the celery vs flask naming conventions being lacking.

think i'd quite like run-celery-with-docker and run-web-with-docker just to be super explicit and unambiguous.

[benthorner]

That makes sense, although I'm not fussed either way myself: as long as it's consistent I think people can get used to it. This would also apply to non-Docker rules i.e. run-celery and run-web (maybe run-flask is clearer?).

Does anyone else have opinions here? Otherwise, we could just go with this.

[CrystalPea]

I think either run-celery and run-flask, or run-async and run-web, as in, it would be nice to have consistent names either after frameworks we use or after functions they perform.

[benthorner]

Closing as all tasks are completed following the merge of https://github.com/alphagov/notifications-python-client/pull/202.* 🎉 🎉 🎉

I think it's worth recapping what the work on this issue has achieved:

• A standard format for all READMEs (or CONTRIBUTING.md for API clients).
• A standard set of tasks in Makefiles, replacing all the old ./scripts/....
• A uniform, flexible approach to developing with Docker, where applicable.
• A common approach to linting - with flake8 and isort - across all repos.
• Parity of which commands we use for local testing and the ones used on CI.
• Stretch goal: a simplified, comprehensive README for our functional-tests.

I applied many of these changes myself and it's been a learning experience 🤓. Although we had to change direction one or two times, I've already benefited from the changes and have had some feedback saying the same ✊. As the newest dev on Notify, I also believe these changes will make the future setup experience significantly simpler ✅. Thanks to everyone who took time to review the many PRs along the way 🙏.

*There is one repo - notifications-certificate-management - that was missed in the original scope and is still inconsistent with all other repos. I think enough work has been done against this issue that it's still reasonable to close it. Since the repo is specific to emergency alerts and quite separate to all the other Notify codebases, it's unlikely anyone new will encounter it - so far existing team members rarely work on it.