Make `ddev ssh` exit more predictable, not outputting useless info, fixes #3738 #4569

DigitalFrontiersMedia · 2023-01-20T21:00:31Z

Attempt to check if values were piped in before deciding to return additional error output from ssh.go. Issue

ddev ssh should return the exit code that is returned from docker-compose and not add extra output #3738.

The Issue

ddev ssh returns output when not needed. It should only do so when a command was piped in.

Lots of confusion on this...

Stop reporting last status from ddev ssh, fixes #1681 #1697 was in 2019... but that code isn't there any more.

The change reversion happened in

Don't let app.Exec() run against non-running container, fixes #1858 #2830

Which was fixing

app.Exec() doesn't check for running site/container #1858

How This PR Solves The Issue

It doesn't. It's a direction that needs work. Reading the stdin/piped-in values removes them from evaluation later down the chain.

Manual Testing Instructions

Should NOT produce additional output and should exit with code 0:

ddev ssh
ls /non-existent
exit
echo $?

SHOULD produce error output from the piped in command:
echo "ls /non-existent" | ddev ssh

Automated Testing Overview

No automated tests at this time.

Release/Deployment Notes

In addition to not solving the problem, ddev ssh "hangs" because it appears to be waiting for stdin input read.

rfay · 2023-01-20T21:25:45Z

You can push new commits into this if you think you see a path forward.

github-actions · 2023-01-20T21:31:30Z

Download the artifacts for this pull request:

rfay · 2023-01-21T16:56:28Z

I took the liberty of rebasing this since the updated docker-compose will be in there now. I don't think it changes anything, but you'll want to update your git checkout.

rfay · 2023-01-21T17:13:16Z

cmd/ddev/cmd/ssh.go

+		if err != nil && isPiped {
 			util.Failed("Failed to ddev ssh %s: %v", serviceType, err)
 		}


The easy part of this PR is to remove the output on failure as it's not useful, and in the case of ddev ssh was already output if needed, and just exit with the exit code, which can be done something like this:

Suggested change

if err != nil && isPiped {

util.Failed("Failed to ddev ssh %s: %v", serviceType, err)

}

if err != nil { // Didn't experiment with your isPiped here...

if exiterr, ok := err.(*exec.ExitError); ok {

os.Exit(exiterr.ExitCode())

} else {

os.Exit(1)

}

}

Interesting. So many moving parts on top of just getting my head around Go, so every little suggestion is appreciated. Will check this out!

rfay · 2023-01-21T17:15:14Z

Oh, if you don't have GoLand (or vscode) set up to let you debug through things, make sure to make that a priority. I love Goland, I think you will too. Lots of people use vscode successfully, but it's more tweaky, and I am not a master of the tweaks. But make sure you can do step-debugging. Of course, the pipe thing you're trying to do here is one of the hardest things to do with step debugging.

DigitalFrontiersMedia · 2023-01-21T17:28:22Z

I'm using VSCode. I'm cash strapped at the moment and I don't see paying for another tool anytime soon. And yeah, I was wondering how you do step debugging with Go. I tried a cursory search but didn't really understand the suggested results. If you have specific suggestions for VSCode, I'm all ears. Will keep looking at these issues as I learn though. Thank you for the opportunity and help!

rfay · 2023-01-21T17:34:32Z

Yeah, IMO the first thing to do with any development project in any language is to get step-debugging going, so spend some time with it. There are docs around, and the Gitpod integration is all set up with it, so you could debug with this PR at https://gitpod.io/#https://github.com/drud/ddev/pull/4569, but since you're an established vscode user I'd try to get it set up locally as well.

rfay · 2023-01-21T18:19:35Z

So see

[BUG] docker-compose exec may return incorrect/wrong return value/exit code docker/compose#10189

It turns out that the phony exit codes are a feature of bash. So all we really need to do here is

Stop outputting any verbiage
Return the exit code as in my suggestion above
Perhaps verify that the target container is running before trying to use it, perhaps something like this:

		err = app.Wait([]string{serviceType})
		if err != nil {
			util.Failed("Service %s doesn't seem to be running: %v", serviceType, err)
		}

DigitalFrontiersMedia · 2023-01-25T21:33:46Z

I'm not sure, but I think this is now doing what you expect?

(base) stephen@MacBook-Pro:dfm-bdw [16:10:22] (master): ddev ssh                 
stephen@dfm-bdw-web:/var/www/html$ ls /xxx
ls: cannot access '/xxx': No such file or directory
stephen@dfm-bdw-web:/var/www/html$ exit
logout
(base) stephen@MacBook-Pro:dfm-bdw [16:15:07] (master): echo $? 
0

(base) stephen@MacBook-Pro:dfm-bdw [16:15:10] (master): echo "ls /xxx" | ddev ssh
ls: cannot access '/xxx': No such file or directory
(base) stephen@MacBook-Pro:dfm-bdw [16:15:15] (master): echo $?                  
2

rfay

Making progress! I think we can get rid of more now :)

I'm going to take the liberty of rebasing this now, because you now have lots of bits and pieces you don't want, so when you start working on it, make sure your git remote is updated.

cmd/ddev/cmd/ssh.go

rfay

IMO this is perfect now. Behavior is as expected. The one thing I'd like to ask is to remove
https://github.com/drud/ddev/blob/995cc6beffe375a5ada8ecdecf17ac707b608ad0/cmd/ddev/cmd/ssh.go#L34-L37

The reason is... we don't have to check siteStatus if the container is running. I think this might be better in the case where one container failed.

DigitalFrontiersMedia · 2023-01-26T20:08:40Z

Yes, I've been meaning all day to say that I think you had it and to thank you for your patience with my tunnel vision as I now totally see what you were getting at the entire time and how what was in the pipe was irrelevant. But I wanted to test it before reporting back on those changes but I seemed to have done something unexplainable that prevents me from running the compiled project. So I haven't been able to test it.

I am using an M1 (arm64), and for some reason (probably from a previous bad install), things were compiling amd64. Oddly, now that I think it's "fixed" I get zsh: killed ddev whenever I try to run the compiled project. But all seems correct:

(base) stephen@MacBook-Pro:~ [14:36:43]: go env GOHOSTOS
darwin
(base) stephen@MacBook-Pro:~ [15:00:04]: go env GOHOSTARCH
arm64
(base) stephen@MacBook-Pro:~ [15:00:08]: which ddev
/usr/local/bin/ddev
(base) stephen@MacBook-Pro:~ [15:00:21]: ls -alh /usr/local/bin/ddev
-rwxr-xr-x  1 stephen  wheel    17M Jan 26 14:38 /usr/local/bin/ddev

That's definitely the executable I just compiled with "make" but still I get zsh: killed ddev.

I've uninstalled and re-installed Go and tools and blew away my local of ddev and re-cloned.

(base) stephen@MacBook-Pro:~ [15:00:34]: go version
go version go1.19.5 darwin/arm64

Don't see how it has anything to do with ddev, so trying to figure out what's wrong and will circle back.

Your signature on this quickfix should be "If things get any worse, I'm going to have to ask you to stop helping." :-D

DigitalFrontiersMedia · 2023-01-26T20:11:05Z

So weird. Compiling for amd64 works:

(base) stephen@MacBook-Pro:dfm-ddev [15:10:16] (master): make darwin_amd64 && cp .gotmp/bin/darwin_amd64/ddev /usr/local/bin/ddev
building .gotmp/bin/darwin_amd64/ddev from ./cmd/... ./pkg/...
(base) stephen@MacBook-Pro:dfm-ddev [15:10:24] (master): ddev version
 ITEM             VALUE                                   
 DDEV version     v1.21.4-57-g1239754d                    
 architecture     amd64                                   
 db               drud/ddev-dbserver-mariadb-10.4:v1.21.4 
 dba              phpmyadmin:5                            
 ddev-ssh-agent   drud/ddev-ssh-agent:v1.21.4             
 docker           20.10.17                                
 docker-compose   v2.15.1                                 
 docker-platform  docker-desktop                          
 mutagen          0.16.0                                  
 os               darwin                                  
 router           drud/ddev-router:v1.21.4                
 web              drud/ddev-webserver:v1.21.4

DigitalFrontiersMedia · 2023-01-26T20:17:23Z

Running amd64, I get the following with the above code, which I believe isn't what you want due to the non-zero exit code on exit:

(base) stephen@MacBook-Pro:dfm-bdw [15:14:25] (master): ddev ssh
stephen@dfm-bdw-web:/var/www/html$ ls /xxx
ls: cannot access '/xxx': No such file or directory
stephen@dfm-bdw-web:/var/www/html$ exit
logout
(base) stephen@MacBook-Pro:dfm-bdw [15:14:39] (master): echo $?
2
(base) stephen@MacBook-Pro:dfm-bdw [15:14:46] (master): echo "ls /xxx" | ddev ssh
ls: cannot access '/xxx': No such file or directory
(base) stephen@MacBook-Pro:dfm-bdw [15:15:03] (master): echo $?                  
2

but I don't really trust my environment right now, maybe it's a Rosetta thing?

DigitalFrontiersMedia · 2023-01-26T20:19:00Z

"The reason is... we don't have to check siteStatus if the container is running. I think this might be better in the case where one container failed."

Yes, I didn't think that was necessary for same reason but you had suggested it earlier and I just thought I was missing something. Will address that after I figure out my local issues.

rfay · 2023-01-26T20:30:25Z

You're doing great, congratulations!

The problem with your build is you have some bad-architecture ddev in your $PATH. Delete them all :) If you do a file $(which ddev) you'll see the architecture of the bad one.

I don't recommend running amd64 anything on arm64. Just confuses everything.

If you just run make it will build ddev into project .gotmp/bin/darwin_arm64/ddev - you can explicitly path that if you want, but I recommend using another technique. I always symlink my /usr/local/bin/ddev to that file.

DigitalFrontiersMedia · 2023-01-26T20:50:03Z

The problem with your build is you have some bad-architecture ddev in your $PATH. Delete them all :) If you do a file $(which ddev) you'll see the architecture of the bad one.

When I run make && cp .gotmp/bin/darwin_arm64/ddev /usr/local/bin/ddev and do as you suggest, I get the following:

(base) stephen@MacBook-Pro:~ [15:39:31]: file $(which ddev)
/usr/local/bin/ddev: Mach-O 64-bit executable arm64
(base) stephen@MacBook-Pro:~ [15:41:37]: ddev version
zsh: killed     ddev version

If you just run make it will build ddev into project .gotmp/bin/darwin_arm64/ddev - you can explicitly path that if you want, but I recommend using another technique. I always symlink my /usr/local/bin/ddev to that file.

Yes, I've just been copying that (as noted above). But if I do make darwin_amd64 && cp .gotmp/bin/darwin_amd64/ddev /usr/local/bin/ddev instead, everything works...but I don't trust it and it makes no sense:

(base) stephen@MacBook-Pro:dfm-ddev [15:41:32] (issue3738): make darwin_amd64 && cp .gotmp/bin/darwin_amd64/ddev /usr/local/bin/ddev
building .gotmp/bin/darwin_amd64/ddev from ./cmd/... ./pkg/...
(base) stephen@MacBook-Pro:dfm-ddev [15:47:54] (issue3738): ddev version
 ITEM             VALUE                                   
 DDEV version     v1.21.4-58-g726740e4                    
 architecture     amd64                                   
 db               drud/ddev-dbserver-mariadb-10.4:v1.21.4 
 dba              phpmyadmin:5                            
 ddev-ssh-agent   drud/ddev-ssh-agent:v1.21.4             
 docker           20.10.17                                
 docker-compose   v2.15.1                                 
 docker-platform  docker-desktop                          
 mutagen          0.16.0                                  
 os               darwin                                  
 router           drud/ddev-router:v1.21.4                
 web              drud/ddev-webserver:v1.21.4

DigitalFrontiersMedia · 2023-01-26T21:06:10Z

Twilight Zone level stuff. Restarted the whole computer. And now seems to make running arm64 versions again (nice loss of a few hours there lol).

That fixed, I don't think that code is working the way you want it to, because if an error is returned from the previous command per os, it spits that out (code 2) and you want 0 on exit:

(base) stephen@MacBook-Pro:dfm-bdw [15:56:21] (master): ddev ssh
stephen@dfm-bdw-web:/var/www/html$ ls /xxx
ls: cannot access '/xxx': No such file or directory
stephen@dfm-bdw-web:/var/www/html$ exit
logout
(base) stephen@MacBook-Pro:dfm-bdw [15:58:34] (master): echo $?
2

This actually worked correctly with the pipe check code in there because if you're not piping anything, it returns nothing:
and I just made the change and recompiled to show the output and getting zsh: killed ddev ssh again.

rfay · 2023-01-26T21:08:26Z

The case you show is bash's fault, and it's OK. As explored in

[BUG] docker-compose exec may return incorrect/wrong return value/exit code docker/compose#10189

I don't think there's a "fix" that makes any sense if that's the behavior of bash. But now we don't whine about it with extra output.

rfay · 2023-01-26T21:10:10Z

Use hash -r when you change that affects your PATH, and I recommend a symlink,
ln -sf <project>/.gotmp/bin/darwin_arm64/ddev /usr/local/bin/ddev

DigitalFrontiersMedia · 2023-01-26T21:15:18Z

I don't think there's a "fix" that makes any sense if that's the behavior of bash. But now we don't whine about it with extra output.

Oh. Okay. Was going off of something you said last week in #3738 (comment):

Not only is the reporting about "failed to ddev ssh web" incorrect, but the return value should actually be 0.

Because if you really want it 0, I was getting that by bypassing that whole return block if we weren't expecting a return value for a piped in command.

rfay · 2023-01-26T21:19:45Z

That was before I studied the bash behavior.

DigitalFrontiersMedia · 2023-01-26T21:26:25Z

Ah! Understood. And I also misread the block you wanted removed. Thought you were asking to remove the service check (not the SiteRunning check).

I'm 2 steps behind but catching up!

DigitalFrontiersMedia · 2023-01-26T21:32:23Z

Give it a look now.

(symlinking was the answer, btw, copying it = bad)

Attempt to check if values were piped in before deciding to return additional error ou tput from ssh.go. Issue ddev#3738.

…g it.

Remove piped input check per rfay suggestion. Co-authored-by: Randy Fay <randy@randyfay.com>

Remove unused iPiped conditional check per rfay suggstion. Co-authored-by: Randy Fay <randy@randyfay.com>

Remove unnecessary siterunning check now that we check for if the service is running.

rfay · 2023-01-26T23:21:50Z

I hope you'll be ready for another challenge! Maybe

variables in provider environment_variables don't get properly evaluated if the value has spaces in it #3499 ?

rfay

Looking great to me! I'll manually test one more time before pull, but congratulations!

DigitalFrontiersMedia · 2023-01-27T16:11:12Z

I hope you'll be ready for another challenge! Maybe

variables in provider environment_variables don't get properly evaluated if the value has spaces in it #3499 ?

Absolutely! Thank you again for the opportunity and the guidance, even that out of scope of the issue. And I'll even name the branch correctly as requested for the PR! :-)

Thank you for the hash -r suggestion, too. I had no idea such a thing existed but fits perfectly with my spidey sense that something akin to a caching problem seemed to be occurring. Feel free to point out other things I'm missing as I go along, such as at this point if I should click "Comment" or "Close with comment" when we've reached a resolution? Also, what are expectations for when in a thread the "Update branch" button should be used?

rfay · 2023-01-27T16:36:12Z

I always try to keep PRs rebased to current upstream/master. This can be done

locally with git fetch upstream && git rebase upstream/master && git push --force-with-lease
Locally by merging upstream/master in. git fetch upstream && git merge upstream/master - I don't ever do this because merge commits hide lots of stuff and I don't like that hidden stuff. But the reality is it works because we always squash PRs here.
On Github web UI with "update branch", again, I always prefer and recommend rebase.

rfay · 2023-01-27T16:37:51Z

I think you're all done here, but general workflow is:

Sometimes start with PR in "draft" mode. Doesn't matter too much.
When it's maturing, change to "ready for review"
When you want review, request review.

When it gets merged, the PR will be closed. If it says "fixes ..." like the title here does, it will auto-close the related issue, #3738 in this case.

rfay

Looks perfect; I did another manual test and things look just right.

It has a great side-effect that I've always wanted. Now when you have a failure to start due to some random container failing, you can still ddev ssh into the other containers that were successfully started.

rfay · 2023-01-27T18:05:24Z

Congratulations!

rfay force-pushed the 3738 branch from b8e447f to 9b47d7f Compare January 21, 2023 16:55

rfay changed the title ~~Check for commands piped into ssh~~ Check for commands piped into ssh, fixes #3738 Jan 21, 2023

rfay reviewed Jan 21, 2023

View reviewed changes

rfay marked this pull request as draft January 25, 2023 15:44

DigitalFrontiersMedia marked this pull request as ready for review January 25, 2023 21:34

rfay requested changes Jan 26, 2023

View reviewed changes

cmd/ddev/cmd/ssh.go Outdated Show resolved Hide resolved

cmd/ddev/cmd/ssh.go Outdated Show resolved Hide resolved

rfay force-pushed the 3738 branch from 9897cc7 to e7941e0 Compare January 26, 2023 01:37

rfay changed the title ~~Check for commands piped into ssh, fixes #3738~~ Make ddev ssh exit more predictable, not outputting useless info, fixes #3738 Jan 26, 2023

rfay requested changes Jan 26, 2023

View reviewed changes

DigitalFrontiersMedia and others added 6 commits January 26, 2023 16:19

Update ssh.go

8e3481f

Attempt to check if values were piped in before deciding to return additional error ou tput from ssh.go. Issue ddev#3738.

Added rfay suggestions. Adjusted code to check stdin without consumin…

37ad472

…g it.

Update cmd/ddev/cmd/ssh.go

e3c15e1

Remove piped input check per rfay suggestion. Co-authored-by: Randy Fay <randy@randyfay.com>

Update cmd/ddev/cmd/ssh.go

09f1e6c

Remove unused iPiped conditional check per rfay suggstion. Co-authored-by: Randy Fay <randy@randyfay.com>

Formatting lint cleanup.

8ca8164

Update ssh.go

a074aaa

Remove unnecessary siterunning check now that we check for if the service is running.

rfay force-pushed the 3738 branch from 0d76620 to a074aaa Compare January 26, 2023 23:19

rfay approved these changes Jan 26, 2023

View reviewed changes

rfay approved these changes Jan 27, 2023

View reviewed changes

rfay merged commit 3081af7 into ddev:master Jan 27, 2023

rfay mentioned this pull request Feb 9, 2023

DDEV v1.21.5 Test Plan #4625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `ddev ssh` exit more predictable, not outputting useless info, fixes #3738 #4569

Make `ddev ssh` exit more predictable, not outputting useless info, fixes #3738 #4569

DigitalFrontiersMedia commented Jan 20, 2023 •

edited by rfay

rfay commented Jan 20, 2023

github-actions bot commented Jan 20, 2023 •

edited

rfay commented Jan 21, 2023

rfay Jan 21, 2023

DigitalFrontiersMedia Jan 21, 2023

rfay commented Jan 21, 2023

DigitalFrontiersMedia commented Jan 21, 2023

rfay commented Jan 21, 2023

rfay commented Jan 21, 2023

DigitalFrontiersMedia commented Jan 25, 2023

rfay left a comment

rfay left a comment

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023 •

edited

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

rfay commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

rfay left a comment

DigitalFrontiersMedia commented Jan 27, 2023

rfay commented Jan 27, 2023

rfay commented Jan 27, 2023

rfay left a comment

rfay commented Jan 27, 2023

-		if err != nil && isPiped {
-			util.Failed("Failed to ddev ssh %s: %v", serviceType, err)
-		}
+		if err != nil { // Didn't experiment with your isPiped here...
+			if exiterr, ok := err.(*exec.ExitError); ok {
+				os.Exit(exiterr.ExitCode())
+			} else {
+				os.Exit(1)
+			}
+		}

Make ddev ssh exit more predictable, not outputting useless info, fixes #3738 #4569

Make ddev ssh exit more predictable, not outputting useless info, fixes #3738 #4569

Conversation

DigitalFrontiersMedia commented Jan 20, 2023 • edited by rfay

The Issue

How This PR Solves The Issue

Manual Testing Instructions

Automated Testing Overview

Related Issue Link(s)

Release/Deployment Notes

rfay commented Jan 20, 2023

github-actions bot commented Jan 20, 2023 • edited

rfay commented Jan 21, 2023

rfay Jan 21, 2023

Choose a reason for hiding this comment

DigitalFrontiersMedia Jan 21, 2023

Choose a reason for hiding this comment

rfay commented Jan 21, 2023

DigitalFrontiersMedia commented Jan 21, 2023

rfay commented Jan 21, 2023

rfay commented Jan 21, 2023

DigitalFrontiersMedia commented Jan 25, 2023

rfay left a comment

Choose a reason for hiding this comment

rfay left a comment

Choose a reason for hiding this comment

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023 • edited

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

rfay commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

DigitalFrontiersMedia commented Jan 26, 2023

rfay commented Jan 26, 2023

rfay left a comment

Choose a reason for hiding this comment

DigitalFrontiersMedia commented Jan 27, 2023

rfay commented Jan 27, 2023

rfay commented Jan 27, 2023

rfay left a comment

Choose a reason for hiding this comment

rfay commented Jan 27, 2023

Make `ddev ssh` exit more predictable, not outputting useless info, fixes #3738 #4569

Make `ddev ssh` exit more predictable, not outputting useless info, fixes #3738 #4569

DigitalFrontiersMedia commented Jan 20, 2023 •

edited by rfay

github-actions bot commented Jan 20, 2023 •

edited

rfay commented Jan 26, 2023 •

edited