docs: Clarify required value is an SSH Destination#334
Conversation
federicobozzini
left a comment
federicobozzini
left a comment
There was a problem hiding this comment.
Minor question but looks good to me.
| t, exists := lookupTarget(cmd) | ||
| if !exists { | ||
| return "", fmt.Errorf("target not specified: provide --target or set TOPO_TARGET env var") | ||
| return "", fmt.Errorf("target not specified: set --target or %s to the SSH destination Topo should connect to (e.g. user@example.local)", targetEnvVar) |
There was a problem hiding this comment.
Are we happy to use connect here? It might suggest some statefulness.
| t, exists := lookupTarget(cmd) | ||
| if !exists { | ||
| return "", fmt.Errorf("target not specified: provide --target or set TOPO_TARGET env var") | ||
| return "", fmt.Errorf("target not specified: set --target or %s to the SSH destination Topo should connect to (e.g. user@example.local)", targetEnvVar) |
There was a problem hiding this comment.
Not a fan of this. I think the correct place to explain the purpose of arguments is --help/usage. We shouldn’t duplicate that information whenever a command fails because an argument wasn't supplied.
Try git clone w/o repo to clone. Or bare docker run.
If we keep it, perhaps make it more conversational: ”where to deploy to? set —target or TARGET_ENV”
The description of —target in usage could definitely be improved though w/ the e.g. you’ve added here.
There was a problem hiding this comment.
Actually, I rescind my comment. I think this is amazing for agentic stuff. We should look for other opportunities to apply this pattern. 👍
th3james
force-pushed
the
improve-unspecified-target-error
branch
from
880eea7 to
b449ce8
Compare
|
@muchzill4 @federicobozzini Realised we were missing the obvious answer here - a short, concise error message, and a revamp of our rather neglected root command description. |
3 participants
Changes
targetis, rather than just yelling that it hasn't been provided