"ddev list" shows sites based on container, even if directory is removed: fix for #374 #444

nmccrory · 2017-08-24T23:27:04Z

The Problem:

A reoccurring issue in ddev list was its failure to list sites whose directories had been removed but containers were still running. This pitfall in ddev list made it possible for containers (which were no longer useful) to heap up on a user's machine if they repeatedly removed sites using rm -r <dir> instead of ddev rm - which stops all containers associated with a particular site.

The Fix:

TLDR; check to see if a site's root directory exists. If it doesn't exist, store information about the site temporarily and list it with status "not found".
ddev list already had visibility into the ddev containers that were running on a machine, it simply would not include sites with missing directories because those sites would raise an error when attempting to initialize from a missing root site.Init(<root_dir>).

The Test:

Start a site
rm -r the site directory
ddev list
- ddev list should list the site with "status: not found"
ddev rm <site_name>
ddev list then docker ps
- the site and it's containers should no longer be visible

Related Issue Link(s):

#374

beeradb · 2017-08-25T15:21:21Z

pkg/plugins/platform/utils.go

+					tempConfig.Name = siteContainer.Labels["com.ddev.site-name"]
+					tempSite := &LocalApp{tempConfig}
+					apps[platformType] = append(apps[platformType], tempSite)
+					break


This break feels kind of weird? what are we breaking from?

This break stops the site variable from running .Init(<app_root>) a few lines below

beeradb · 2017-08-25T15:23:03Z

pkg/plugins/platform/utils.go

@@ -197,3 +210,15 @@ func ddevContainersRunning() (bool, error) {
 	}
 	return false, nil
 }
+
+// exists returns whether the given file or directory exists or not
+func exists(path string) (bool, error) {


Please use the FileExists from our fileutils package: https://github.com/drud/ddev/blob/master/pkg/fileutil/files.go#L119

It's already imported here, so it should be an easy update.

beeradb

I ran through this and it looks like it breaks some existing functionality.

Here's what I did

Started two sites. "drudio" and "ddevd7"
Run a ddev list (master) to ensure both were shown in the output
Deleted the "drudio" folder.
Run ddev list. Observed only a "ddevd7" site.
Compile this PR. Run ddev list. Observered only "drudio" listed. It showed the deleted folder "~/Projects/testing/drudio"
Checked out master again
Run ddev list on master. Observed only "ddevd7" listed.

tl;dr: When a site is deleted it appears that this PR only shows the deleted site. Or perhaps just the first site.

Console output:

drudapi/docker.nginx-php-fpm-hosting git:(master) ✗ » ddev list
2 local sites found.
NAME    TYPE       LOCATION                   URL                       STATUS
drudio  wordpress  ~/Projects/testing/drudio  http://drudio.ddev.local  running
ddevd7  drupal7    ~/Projects/ddev/ddevd7     http://ddevd7.ddev.local  running

DDEV ROUTER STATUS: running
drudapi/docker.nginx-php-fpm-hosting git:(master) ✗ » z ddev
drud/ddev git:(nginx-tag) » hub checkout https://github.com/drud/ddev/pull/444/files
remote: Counting objects: 12, done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 12 (delta 6), reused 12 (delta 6), pack-reused 0
Unpacking objects: 100% (12/12), done.
From git://github.com/nmccrory/ddev
 * [new branch]      ddev-list-rmdir -> nmccrory/ddev-list-rmdir
Branch nmccrory-ddev-list-rmdir set up to track remote branch ddev-list-rmdir from nmccrory.
Switched to a new branch 'nmccrory-ddev-list-rmdir'
drud/ddev git:(nmccrory-ddev-list-rmdir) » rm -rf ~/Projects/testing/drudio
drud/ddev git:(nmccrory-ddev-list-rmdir) » ddev list
1 local site found.
NAME    TYPE     LOCATION                URL                       STATUS
ddevd7  drupal7  ~/Projects/ddev/ddevd7  http://ddevd7.ddev.local  running

DDEV ROUTER STATUS: running
drud/ddev git:(nmccrory-ddev-list-rmdir) » make darwin
building darwin from ./cmd/... ./pkg/...
drud/ddev git:(nmccrory-ddev-list-rmdir) » ddev list
1 local site found.
NAME    TYPE  LOCATION                   URL                       STATUS
drudio        ~/Projects/testing/drudio  http://drudio.ddev.local  web service unhealthy

DDEV ROUTER STATUS: running
drud/ddev git:(nmccrory-ddev-list-rmdir) » git checkout master
drud/ddev git:(master) » make darwin
building darwin from ./cmd/... ./pkg/...
drud/ddev git:(master) » ddev list
1 local site found.
NAME    TYPE     LOCATION                URL                       STATUS
ddevd7  drupal7  ~/Projects/ddev/ddevd7  http://ddevd7.ddev.local  running

nmccrory · 2017-08-25T16:19:17Z

Thanks for testing this and giving your feedback @beeradb. I'll investigate into what's causing the issue (maybe the break statement as you mentioned above).

rickmanelius · 2017-08-25T20:42:46Z

@nmccrory @beeradb I've followed the conversation in our team Slack channel. I believe the summarized approach is that ddev list should still show containers that persist even when the directory has been removed. However, there are two possible scenarios:

The codebase is still intact, but the .ddev folder has been removed.
The codebase is removed.

In both cases, we should probably note the issue in the status column and (at the bottom of the ddev list output) provide instructions how to either resolve OR remove the containers. Is that the summarized proposal? Right now, the PR summary mentions a status of "not found", but it doesn't indicate what instructions or helper text would be provided to help resolve the detached containers.

rfay · 2017-08-30T23:15:57Z

If you could just push a dummy commit into this it should pass surf-windows @nmccrory

rfay · 2017-08-31T22:13:28Z

@nmccrory could you comment on why this couldn't be handled in the status field, the same as what happens when a particular service hasn't come up or has been disabled? I know you explored that, just don't understand why it didn't work out.

nmccrory · 2017-09-06T20:21:56Z

pkg/plugins/platform/types.go

@@ -46,7 +49,7 @@ var PluginMap = map[string]App{
 }

 // GetPluginApp will return an application of the type specified by pluginType
-func GetPluginApp(pluginType string) (App, error) {
+func GetPluginApp(pluginType string) (*LocalApp, error) {


Please consider this piece rather heavily. This works best now because it is the only way I could add the InitFromMissingDirectory(string, string) method on type LocalApp, without defining it as a required part of the App interface.

In the future, if we deal with other App types that are not LocalApp we would need to revert to returning the App generic. Right now however, the only possible return value is LocalApp. I'm not sure what our long term plans are or if there are situations where this may not be the case. Can definitely use feedback here...

I'd say don't do it and don't bother with adding the InitFromMissingDirectory(), as elsewhere.

nmccrory · 2017-09-06T22:35:05Z

This PR is ready for review.

rfay

Minor changes requested - Just lose the extra function and do those 2 lines inline. Add some comments, etc.

You probably need to test the other commands. For example, ddev describe TestMainPkgWordpress doesn't work here, neither does ddev rm TestMainPkgWordpress or ddev stop TestMainPkgWordpress or ddev logs TestMainPkgWordpress, which people will want. Maybe should also be fixed at this time? My bet is they're pretty easy given what you've already learned. Probably it just means adding your general approach to those. Or maybe they can't be done. I'm ok if it's impossible, but it would be nice to see the general case solved.

rfay · 2017-09-06T23:29:07Z

pkg/plugins/platform/utils.go

+				if err != nil {
+					siteName := siteContainer.Labels["com.ddev.site-name"]
+					appType := siteContainer.Labels["com.ddev.app-type"]
+					site.InitFromMissingDirectory(siteName, appType)


I don't think it's worth adding the function here. There are only 2 lines in the function, and it probably has a misleading name. So

just take those 2 lines and put them here.

Add a comment for this stanza saying what the situation is.

Also, make sure there aren't other reasons that the site.Init() might fail that would cause misbehavior in this stanza. (Like do we have a siteContainer OK?)

I'm just saying do this:

if err != nil { siteName := siteContainer.Labels["com.ddev.site-name"] appType := siteContainer.Labels["com.ddev.app-type"] site.AppConfig.Name = siteName site.AppConfig.AppType = appType log.Printf("Failed to init %v err=%v", approot, err) }

Unfortunately in this case, we can't access the site's AppConfig from this context. This has something to do with the way we are using the App interface, but regardless we need to brainstorm because I agree that it should not have to be its own function...

Brainstorm successful: we can work around this situation by casting site as type LocalApp. From there you can edit values in .AppConfig.

rfay · 2017-09-06T23:29:46Z

pkg/plugins/platform/local.go

@@ -47,6 +47,7 @@ func (l *LocalApp) GetType() string {
 func (l *LocalApp) Init(basePath string) error {
 	config, err := ddevapp.NewConfig(basePath, "")
 	if err != nil {
+		l.AppConfig = config


Add a comment above this saying why we're saving it away, so somebody doesn't remove this someday.

rfay · 2017-09-06T23:30:51Z

pkg/plugins/platform/types.go

@@ -46,7 +49,7 @@ var PluginMap = map[string]App{
 }

 // GetPluginApp will return an application of the type specified by pluginType
-func GetPluginApp(pluginType string) (App, error) {
+func GetPluginApp(pluginType string) (*LocalApp, error) {


I'd say don't do it and don't bother with adding the InitFromMissingDirectory(), as elsewhere.

…nually using .

…xt to our changes using comments.

nmccrory · 2017-09-12T16:03:49Z

@rfay Ready for review

rfay

I think this is ready, please make the one fix I requested. I don't care about the variable name as long as it doesn't shadow "site".

rfay · 2017-09-12T16:35:53Z

pkg/plugins/platform/utils.go

-					apps[platformType] = append(apps[platformType], site)
+				if err != nil {
+					// Cast 'site' from type App to type LocalApp, so we can manually enter AppConfig values.
+					site := site.(*LocalApp)


The cast returns 2 args, the struct (if it exists) and "ok" that tells you whether it existed. I'd rather see

siteStruct, ok := site.(*LocalApp) if (ok) { siteStruct.AppConfig.Name = siteName siteStruct.AppConfig.AppType = appType }

There are two issues here:

Catch the case where the cast doesn't work (unlikely, but defensive code is good)

Don't change the meaning of the variable "site" in the middle of the code. It was one thing, and changing it to something completely different could throw somebody working on this in the future. Never forget that this has a life after you work on it.

…iable storing the casted value.

nmccrory · 2017-09-12T16:49:14Z

Requested changes have been made. Thanks for your feedback, Randy! @rfay

rfay · 2017-09-12T16:56:56Z

pkg/plugins/platform/utils.go

+					// Cast 'site' from type App to type LocalApp, so we can manually enter AppConfig values.
+					siteStruct, ok := site.(*LocalApp)
+					if !ok {
+						log.Fatalf("Failed to init %v err=%v", approot, err)


I don't mind the path you took here, but the error message makes no sense. Please make it make sense. It needs to be about the failure of the cast.

In this context I feel we would want this error to be the one presented. It is more important than the failure to cast because casting only happens when we can't find a config in the app's root directory. If we can't resolve the missing directory issue by casting, we want to return the overall error that the cast is nested in.

I think presenting the failure to cast message on the line before the log.Fatalf would be a good compromise.

beeradb

This looks really close. I think we need one more conditional to check for the absence of a ddev config as well. My original testing method was just to move my .ddev folder after starting, and no errors were reported.

If we can get that other check in place I think we'll be 👍

beeradb · 2017-09-12T19:36:58Z

pkg/plugins/platform/local.go

@@ -266,6 +268,10 @@ func (l *LocalApp) SiteStatus() string {
 	var siteStatus string
 	services := map[string]string{"web": "", "db": ""}

+	if !fileutil.FileExists(l.AppRoot()) {


This only notifies a user if the entire AppRoot is missing. The case where just a .ddev/config.yaml file is missing is also an issue. It would be great to add another check here (and error return) if the config can't be found.

nmccrory · 2017-09-12T20:31:08Z

@beeradb - your feedback has been incorporated. ddev list now changes the site status if the site is missing a .ddev/config.yml file!

rfay · 2017-09-12T21:52:14Z

@beeradb if you're good with this we should let him get it in because the followup work uses some of these things.

beeradb

This worked as expected for me:

drud/ddev ‹minikube› git:(nmccrory-ddev-list-rmdir) » ddev list
1 local site found.
NAME         TYPE     LOCATION                        URL                            STATUS
d7kickstart  drupal7  ~/Projects/testing/d7kickstart  http://d7kickstart.ddev.local  .ddev/config.yml missing

I would note that a full path would probably be preferable under status, but I don't see a need to hold this up for that. It might be nice to add the full path to the output in one of the follow-ups to this, though.

Thanks so much for the work on this. Here's a baloon🎈

nmccrory requested review from beeradb and tannerjfco August 24, 2017 23:27

beeradb reviewed Aug 25, 2017

View reviewed changes

beeradb suggested changes Aug 25, 2017

View reviewed changes

nmccrory mentioned this pull request Aug 25, 2017

"ddev list" should list sites based on container even if directory removed #374

Closed

rfay mentioned this pull request Sep 3, 2017

Socket.io base ddev/ddev-ui#20

Closed

nmccrory force-pushed the ddev-list-rmdir branch from adfb4ee to 2f231bd Compare September 6, 2017 20:17

nmccrory commented Sep 6, 2017

View reviewed changes

rfay assigned nmccrory Sep 6, 2017

rfay requested changes Sep 6, 2017

View reviewed changes

nmccrory added 3 commits September 7, 2017 16:57

Add site status for directory removed

85f4f2d

In the case of a missing site directory, initalization can be done ma…

2267828

…nually using .

Remove erraneous InitFromMissingDirectory function and add some conte…

e3c5544

…xt to our changes using comments.

nmccrory force-pushed the ddev-list-rmdir branch from 2f231bd to e3c5544 Compare September 7, 2017 22:33

Cast site to *LocalApp.

eae481c

rfay approved these changes Sep 12, 2017

View reviewed changes

Handle potential errors while casting using . Use better name for var…

cf31f19

…iable storing the casted value.

rfay reviewed Sep 12, 2017

View reviewed changes

nmccrory added 2 commits September 12, 2017 12:15

Adjusted error message presented on failure to cast.

d0a2d9b

Present value of site instead of siteStruct.

4c60861

beeradb suggested changes Sep 12, 2017

View reviewed changes

Handle the case of a missing .ddev/config.yml file

e3f0140

beeradb approved these changes Sep 12, 2017

View reviewed changes

nmccrory merged commit d47345f into ddev:master Sep 12, 2017

rfay mentioned this pull request Oct 24, 2017

Regression: ddev list, with appdir deleted, gives a panic #521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"ddev list" shows sites based on container, even if directory is removed: fix for #374 #444

"ddev list" shows sites based on container, even if directory is removed: fix for #374 #444

nmccrory commented Aug 24, 2017

beeradb Aug 25, 2017

nmccrory Aug 25, 2017

beeradb Aug 25, 2017 •

edited

beeradb left a comment

nmccrory commented Aug 25, 2017

rickmanelius commented Aug 25, 2017

rfay commented Aug 30, 2017

rfay commented Aug 31, 2017

nmccrory Sep 6, 2017

rfay Sep 6, 2017

nmccrory commented Sep 6, 2017

rfay left a comment

rfay Sep 6, 2017 •

edited

nmccrory Sep 7, 2017

nmccrory Sep 7, 2017

rfay Sep 6, 2017

rfay Sep 6, 2017

nmccrory commented Sep 12, 2017

rfay left a comment

rfay Sep 12, 2017

nmccrory commented Sep 12, 2017

rfay Sep 12, 2017

nmccrory Sep 12, 2017

nmccrory Sep 12, 2017

beeradb left a comment

beeradb Sep 12, 2017

nmccrory commented Sep 12, 2017

rfay commented Sep 12, 2017

beeradb left a comment

"ddev list" shows sites based on container, even if directory is removed: fix for #374 #444

"ddev list" shows sites based on container, even if directory is removed: fix for #374 #444

Conversation

nmccrory commented Aug 24, 2017

The Problem:

The Fix:

The Test:

Related Issue Link(s):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeradb Aug 25, 2017 • edited

Choose a reason for hiding this comment

beeradb left a comment

Choose a reason for hiding this comment

nmccrory commented Aug 25, 2017

rickmanelius commented Aug 25, 2017

rfay commented Aug 30, 2017

rfay commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmccrory commented Sep 6, 2017

rfay left a comment

Choose a reason for hiding this comment

rfay Sep 6, 2017 • edited

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

nmccrory commented Sep 12, 2017

rfay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmccrory commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beeradb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmccrory commented Sep 12, 2017

rfay commented Sep 12, 2017

beeradb left a comment

Choose a reason for hiding this comment

beeradb Aug 25, 2017 •

edited

rfay Sep 6, 2017 •

edited