craftcms / nitro

Speedy local dev environment for @craftcms.
https://getnitro.sh
MIT License
178 stars 24 forks source link

`remove` command does not remove container without running `apply` a second time #321

Closed mattstein closed 3 years ago

mattstein commented 3 years ago

Description

After running nitro remove you have to run apply again to see the container removed even in you answer Y for Apply changes now [Y/n]?.

Notice the (just-added) Removing test.nitro here:

❯ nitro remove
Select a site:
  1. craftcms.nitro
  2. europa.nitro
  3. starterblog.nitro
  4. test.nitro
  5. tutorial.nitro
Enter your selection: 4
Removing test.nitro
Apply changes now [Y/n]? Y
Checking network…
  ✓ network ready
Checking proxy…
  ✓ proxy ready
Checking databases…
  … checking mysql-5.7-3306.database.nitro ✓
  … checking postgres-12-5432.database.nitro ✓
Checking services…
  … checking dynamodb service ✓
  … checking mailhog service ✓
  … checking redis service ✓
Checking containers...
  … checking bitnami.containers.nitro ✓
Checking sites…
  … checking craftcms.nitro ✓
  … checking europa.nitro ✓
  … checking starterblog.nitro ✓
  … checking tutorial.nitro ✓
Checking proxy…
  … updating proxy ✓
Updating hosts file (you might be prompted for your password)

Yet the apply output doesn’t include a Cleaning up... step that removes the container. It’s still there:

❯ nitro ls
Hostname                                 Type              Status
bitnami.containers.nitro                 custom            running
craftcms.nitro                           site              running
europa.nitro                             site              running
mysql-5.7-3306.database.nitro            database          running
nitro-proxy                              proxy             running
postgres-12-5432.database.nitro          database          running
redis.service.nitro                      site              running
starterblog.nitro                        site              running
test.nitro                               site              running
tutorial.nitro                           site              running

But running apply again does what you’d expect:

❯ nitro apply
Checking network…
  ✓ network ready
Checking proxy…
  ✓ proxy ready
Checking databases…
  … checking mysql-5.7-3306.database.nitro ✓
  … checking postgres-12-5432.database.nitro ✓
Checking services…
  … checking dynamodb service ✓
  … checking mailhog service ✓
  … checking redis service ✓
Checking containers...
  … checking bitnami.containers.nitro ✓
Checking sites…
  … checking craftcms.nitro ✓
  … checking europa.nitro ✓
  … checking starterblog.nitro ✓
  … checking tutorial.nitro ✓
Checking proxy…
  … updating proxy ✓
Cleaning up...
  … removing test.nitro ✓
Nitro is up and running 😃

I would expect behavior consistent with nitro add, where if you’ve chosen to Apply changes now the container would be cleaned up + removed immediately. It seems unintuitive that you’d have to run apply again to achieve the desired effect.

Steps to reproduce

  1. Use nitro remove and choose a valid site to remove.
  2. Answer Y (or accept default) to Apply changes now.
  3. Observe that chosen site is removed from nitro.yaml, but its container is still running and chained apply command output did not include cleanup step.
  4. Run nitro apply and observe that container is removed and included in cleanup step output.

Additional info

mattstein commented 3 years ago

FWIW I’ve just realized this is also true for nitro container remove.

mattstein commented 3 years ago

Note to future self: update this article when the fix for the issue is released.

jasonmccallister commented 3 years ago

@mattstein just fixed this in https://github.com/craftcms/nitro/commit/a086070219a2dae5904a06a8618486d1e7963bc2

Any command that called apply after would have been impacted.

mattstein commented 3 years ago

Built locally and confirm joy with nitro container remove. 👍

jasonmccallister commented 3 years ago

Released with v2.0.8.