feat(ssh): select an instance by number or with any - #1149
Conversation
When several instances are running, clever ssh currently requires an interactive prompt (and fails in non-interactive mode). Accept --instance <instance-id> to select the target directly, as requested in #1067. Instance IDs are used rather than instance numbers, which are not stable across deployments. Fixes #1067
|
🔎 A preview has been automatically published! If you created the alias to the preview script, you can run this command to download and install this preview: clever-preview update davlgd-ssh-instance-selectionYou can also run it from your local repository: ./scripts/preview.js update davlgd-ssh-instance-selection
This preview will be deleted once this PR is closed. |
pdesoyres-cc
left a comment
pdesoyres-cc
left a comment
There was a problem hiding this comment.
Few comments below. and I'm wondering if the private functions should be below the main command handler.
| // Every line carries its own indent: `Logger.error` indents continuation lines by the width of | ||
| // its prefix, so a list indented once by the template would come out with its first line two | ||
| // columns further right than the others. |
| return readiest(byNumber); | ||
| } | ||
|
|
||
| const found = byNumber.find((inst) => inst.id === wanted) ?? readiest(byNumber.filter(hasNumber(wanted))); |
There was a problem hiding this comment.
optimization and readability:
The wanted to valid number logic is made on every items of the array while filtering.
Prefer do the parsing/validation once, stop early and then filter with a valid wanted number.
There was a problem hiding this comment.
Now parsed once into a number, with an early return when it isn't one, so the filter only ever runs on a valid value.
| throw new Error( | ||
| `No instance ${styleText('red', wanted)} on this application, pick one of:\n${styleText('grey', available)}`, | ||
| ); |
There was a problem hiding this comment.
I would have make this function just select the right instance and return null if none is found.
Throwing and error with the right message is more a command concern (should be inside the command handler function)
There was a problem hiding this comment.
Agreed. selectInstance returns null now and the handler raises the error, which also let the sort move up and be shared with the prompt.
fa64e69 to
addaf19
Compare
Moved below the handler |
3 participants
Builds on #1123 by @mehdi653, whose commit is included here — the
--instanceoption and itsresolution by ID are theirs.
Context
As discussed, that option should accept instance numbers as well as IDs.
Numbers turn out not to be unique, though. During a redeploy the instance going away and the one
coming up carry the same number:
--instance 0would then reach either, and the API listed the outgoing one first.Changes
anyin any case, alongside the ID.UPinstance:anylooks for one before falling back to the lowest number, and anumber prefers the
UPinstance among those sharing it. An ID still names one entry whatever itsstate, so a stopping instance stays reachable when asked for explicitly.
--instance --command "hostname"says what is wrongrather than reporting an instance named nothing.
How to review
selectInstanceis the whole change; the rest is the option and its documentation.Scale an application to two instances and check that
--instance <number>reaches the one thatnumber names — the remote
hostnameis the instance ID, so the output says which one answered.Then redeploy and, while both instances share number 0, check that
--instance 0and--instance anyreach the one that isUP.