Conversation
Add command that will run on the remote exec. By default the ssh session will detach and leave the command running using nohup. The Pid of the command will be stored in pid.nohup file. We can use this later to stop the exec command if we need to. Include the `--interactive`/`-i` flag to stay attached to the remote after running the exec command. Also: - share some of the connection logic between SSH and Exec commands by introducing an interface that allows the two commands to differ.
Remove the session-id from the exec command, so that exec command will always create a new exec. Pass the command to the api to be stored along with the details of the exec. Also - Wrap the user's command in `bash -c` allowing the user to input multiple commands in a single string. e.g. `cd somedir && ./serve.py`
noorvir
left a comment
There was a problem hiding this comment.
Looks mostly good! Once we clarify the onTerminate happy to merge
| return err | ||
| } | ||
| func (e *execCommandFlow) onTerminate(ctx context.Context, execID string) error { | ||
| ui.Infof("Session %q exited. Use 'unweave terminate' to stop the session.", execID) |
There was a problem hiding this comment.
Since the exec is a command running in a container, it should automatically exit when the command exits right? So we shouldn't need this.
There was a problem hiding this comment.
At this point, we don't know if the ssh session terminated because the command was run detached, or the ssh session terminated because the command was running attached and finished.
Another thought is that out exec logs currently exist inside the exec, and no where else. So if you want to read the logs after the command has completed, you need to keep the exec around.
While this logs problem (and the above ssh problem) still exist, I recon we might just ask the user to terminate manually (maybe?).
Both of these problems are solvable, but maybe we can ship this improvement first. 🤔
| GroupID: groupDev, | ||
| Hidden: true, | ||
| Short: "Execute a command serverlessly", | ||
| Use: "exec [flags] -- [<command>]...", |
There was a problem hiding this comment.
You shouldn't need to add the [flags] manually here. Cobra will do it for you (I think)
There was a problem hiding this comment.
This Use overrides the default that Cobra will do (where it includes flags), but I wanted to include the [command] section, which I didn't find an in-built way in Cobra to do.
Do you know one? Or I can leave it like this?
There was a problem hiding this comment.
Oh very interesting! Cobra automatically matches the [flags] and if you've provided it, it doesn't append it to the end! Very cool! I didn't know that. In this is perfect and correct
| if err := cmd.Run(); err != nil { | ||
| return fmt.Errorf("failed to add: %s %s", stderr.String(), stdout.String()) | ||
| if doubleDashIdx == -1 { | ||
| const errMsg = "❌ Invalid arguments. You must pass the -- flag. " + |
There was a problem hiding this comment.
This is probably fine for now but I don't necessarily need to constrain the user to always use --. If they don't have any flags to pass to their sub command, they should be able to use unweave exec python script.py. This also looks nicer in demos. I believe Docker, Doppler, etc also do this.
There was a problem hiding this comment.
Yea, I agree that it's visually much more pleasing to see the unweave exec python script.py, and you're right docker does it like this.
I based this on kube:
From the kubectl exec -h printout:
Examples:
# Get output from running the 'date' command from pod mypod, using the first container by default
kubectl exec mypod -- date
I don't have a concrete example for when -- is required, but I can imagine there's a case where we might confuse our flags with the user's flags. I'm guessing that this -- separator will protect us from the potential difficulty in the future. And IMO it's a relatively small cost of the cli user (?). Thoughts?
There was a problem hiding this comment.
The -- is the "end of options" delimiter. i.e. any flags passed after this are not parsed by cobra. We shouldn't have any issues with the user not passing the -- flag because cobra will automatically complain if the flag isn't defined in the command spec. eg, if the user does:
unweave exec python script.py --user-flag=1
Cobra will complain the --user-flag isn't defined. We automatically get this validation.
What we can do is that we don't advertize the fact that you don't need to use -- but if a power user is familiar with it, decides to run unweave exec python train.py we shouldn't throw an error. WDYT?
There was a problem hiding this comment.
I think this is good to merge for now though. We can remove this check later on if needed
Add command that will run on the remote exec.
By default the ssh session will detach and leave the command running using nohup. The Pid of the command will be stored in pid.nohup file. We can use this later to stop the exec command if we need to.
Include the
--interactive/-iflag to stay attached to the remote after running the exec command.Also:
Example:
unweave exec -p 8080 -- python3 -m http.server 8080