kitware-resonant / cookiecutter-resonant

Apache License 2.0
11 stars 6 forks source link

Update docker-compose.yml #183

Closed subdavis closed 1 year ago

subdavis commented 2 years ago

Allow users of this cookiecutter to choose whether they use docker-compose down or docker-compose stop based on their individual needs without losing their database and files.

@zachmullen asked me to make this change a while ago, but I never got around to it.

brianhelba commented 2 years ago

The README docs already state:

When finished, run docker-compose stop

The commands down and stop do very different things. Running docker-compose down -v already has a purpose: it allows a fresh reset of the development environment state, including the database and MinIO.

If this change is made, it requires a separate command to destroy the persistent storage.

I'm not clear on why you would want to destroy the postgres container, but not destroy the postgres volume. Is there a use case for this?

brianhelba commented 2 years ago

Also, if we did make this change, we should include rabbitmq. The AMQP queue is just as important to guarantee persistence of as the database and S3.

manthey commented 2 years ago

One reason to destroy a database container but not a volume is to upgrade the version of the database image.

Another reason to destroy and recreate containers is to expose/remove additional ports or add/remove additional volumes. For instance, you don't generally need to expose the database port outside of the compose network, but it can be handy to do so for some experimention.

Why would you ever want non-persistent storage of a database container to be the default for anything other than CI testing?

subdavis commented 2 years ago

When finished, run docker-compose stop

That's kinda vague language, I think we should reword it to tell the user how to stop their stack vs. how to destroy their stack. There are reasons you'd want to do both: either switch projects or clean the slate and start over with your current project.

I'm not clear on why you would want to destroy the postgres container, but not destroy the postgres volume. Is there a use case for this?

In addition to the things David listed, I like to use docker system prune to clean my docker environment without worrying that it will be destructive to application data. When I want to destroy application data, I like the intentional inclusion of the -v argument rather than having that behavior be implicit.

Also, if we did make this change, we should include rabbitmq. The AMQP queue is just as important to guarantee persistence of as the database and S3.

Great point, we should do that.

subdavis commented 2 years ago

Added Rabbit and a documentation line. PTAL.

brianhelba commented 2 years ago

For instance, you don't generally need to expose the database port outside of the compose network, but it can be handy to do so for some experimention.

This seems like an atypical use case for most routine development. It's also dangerous, since the database doesn't have strong credentials.

Downstream projects or developers are free to enable this capability, but I don't think it's worth the risk or complexity to include by default.

subdavis commented 2 years ago

I didn't make those changes on this PR. We should debate them on another PR if someone wants to make it.

brianhelba commented 2 years ago

Why would you ever want non-persistent storage of a database container to be the default for anything other than CI testing?

clean my docker environment without worrying that it will be destructive to application data. When I want to destroy application data, I like the intentional inclusion of the -v argument rather than having that behavior be implicit.

As a philosophical matter, I think an ephemeral development environment is a feature, not a bug. I expect that there will be significant disagreement on this, but it's an area where I want this cookiecutter to have an opinionated stance. Of course, as with all default choices, downstreams may change things however they'd like.

My argument is that projects should be in the habit of maintaining a reproducible development environment at all times. A snowflake development environment has many risks:

  1. Any valuable data should not uniquely live on a personal development machine. It should be part of a production database or data store and be backed up as part of good operations practices. Developers often break their own machines, for a variety of reasons (often unrelated to the project in question).
  2. To easily reproduce bugs, development environments should be generic. If your local machine is special, you're violating https://12factor.net/dev-prod-parity
  3. It should be fast and easy to spin up new development environments. A difficult bootstrap procedure makes it difficult to onboard new developers and makes it scary to run destructive local experiments.

Realistically, I realize that some projects may not have a distinct production server, and may using a developer's own machine to host important data or serve demos to external machines. I have seen this happen on many Girder 3 projects. For such cases, I do not think that this cookiecutter is a good fit, as it is heavily tuned towards supporting production-first use cases, not research demos.

subdavis commented 2 years ago

While this change seems small, I think it actually has important effects. Also I enjoy arguing with you :)

I realize that some projects may not have a distinct production server

This change is not intended to support running a service in production. This is just for the developer.

I think an ephemeral development environment is a feature, not a bug.

I don't agree. We should make this change because the change allows us both to do what we individually prefer, whereas not making this change encourages what I think is a bad habit.

I don't like constantly resetting my environment because it allows me to get into a mindset that can result in thinking and problem-solving that are either 1) overfitted to whatever data is part of the auto-population script (this happens to me on DIVE) or 2) overfitted to whatever data is conveniently available (probably in your ~/downloads directory), like that 1 spreadsheet the customer gave you. You have to log into box.com to search for more examples, which sounds like work, so you just keep using the one you have (happens to me to some extent on every project, but VIIME is a good example).

If you only ever look at 3 pre-loaded data exemplars (and you think like me, which, IDK, maybe you don't), you start to develop a bias where you think all data is going to look like those 3 exemplars and, inevitably, it doesn't.

maintaining a reproducible development environment at all times

Agree. Think of this change as turning on the cache. With persistence enabled, state is still reproducible. It just means I don't have to waste time constantly reproducing it.

To easily reproduce bugs, development environments should be generic

A corollary: To easily discover bugs, a bit of noise and mess in your dev environment is actually desirable. I like to keep lots of weird edge case data in my environment. When hunting a particular bug, I often make intentionally weird data, and then I just leave it to rot. Having this data just floating around and getting stuck in the gears is a useful way to accidentally find bugs, or to remind you to add robustness when working on features.

In an idea world, the most useful of this data would get formalized into unit tests or added to auto-population, but we do not live in that world and I often do not have time for that.

Any valuable data should not uniquely live on a personal development machine.

Agree.

It should be fast and easy to spin up new development environments.

This change does not add a difficult bootstrap procedure.

TL;DR

This is intended to change the default to allowing data to accumulate in your development environment, increasing the odds that at any given time, you have a greater diversity of sample data (and some junk data) to inform your thinking in feature development and to just generally clutter up the place. Data is messy, and production is messy, so your dev environment should be messy sometimes too.

zachmullen commented 2 years ago

FWIW I prefer the persistent volumes. +1 for this change.

subdavis commented 2 years ago

Bump, could this be either merged or rejected? Thanks!

banesullivan commented 2 years ago

+1 for this.

Persistent volumes and thus persistent test data and user accounts etc. should be our standard.

This is mostly about convenience for me. I end up adding these same changes anytime I develop on a Girder4 project so that I can destroy containers and upgrade packages etc. without re-running (sometimes expensive) data ingest routines.

subdavis commented 2 years ago

Closing out remaining PRs, but looks like this one is approved so I'll give it a couple days if you still want to merge it.

zachmullen commented 2 years ago

I find this the most compelling argument:

A corollary: To easily discover bugs, a bit of noise and mess in your dev environment is actually desirable. I like to keep lots of weird edge case data in my environment. When hunting a particular bug, I often make intentionally weird data, and then I just leave it to rot. Having this data just floating around and getting stuck in the gears is a useful way to accidentally find bugs, or to remind you to add robustness when working on features.

This truly mirrors a long-running production environment, filled with old data from multiple migrations ago. I really put a lot of value in that. @brianhelba can we merge this, or do you still object?

brianhelba commented 2 years ago

@banesullivan and I discussed this recently, and I have a path forward to merge this after some changes. Please don't close or merge this yet.

banesullivan commented 1 year ago

@brianhelba, did you ever work on this? I'm still +1 for this merging

zachmullen commented 1 year ago

Also still +1 from me. It just seems natural to attach a volume to anything that involves persistence. On multiple projects now I've seen people use the development docker-compose script as a starting point to translate to a prod script, and having the volumes listed is helpfully expressive for that scenario. @brianhelba if we properly document stopping vs. destroying would you be OK with this?

zachmullen commented 1 year ago

I'm going to merge this since there seems to be no more objection.

zachmullen commented 1 year ago

Superseded by #199