Add "remote actions" feature #515
Conversation
Signed-off-by: Andy Doan <andy@foundries.io>
|
I was hesitant to pull in the nanoid stuff, but it does make the tests a lot more readable (better than b32 in my opinion). You can see before(regular UUID) and after here: |
| Use: "trigger", | ||
| Short: "Trigger remote actions on device", | ||
| Long: ` | ||
| *NOTE*: Requires devices running LmP version 97 or later. |
There was a problem hiding this comment.
Long help usually starts with the same line as the short help (without a leading newline). They are not being concatenated.
|
|
||
| func init() { | ||
| triggerCmd := &cobra.Command{ | ||
| Use: "trigger", |
There was a problem hiding this comment.
Can we use something like action or run or exec?
I find the word trigger rather cryptic in this context.
TBH the most natural choice would be to do fioctl device actions [list] to list available remote actions, and fioctl device [actions] run|exec to run an action. I'm not sure if we need the actions keyword for the latter, it depends. If we also want something like fioctl device actions tail, then it would probably be handy to group all these commands. Also, I'm not sure if it is possible to show the user currently running actions... probably possible, right?
|
|
||
| ccr := opts.AsConfig() | ||
| subcommands.DieNotNil(d.Api.PatchConfig(ccr, false)) | ||
| fmt.Println("Config change submitted. Command ID is:", opts.CommandId) |
There was a problem hiding this comment.
Can we print here how the user can see the command log/result? Is there a fioctl command for that?
| } | ||
| } | ||
| fmt.Println("Remote actions are not configured for this device") | ||
| os.Exit(0) |
There was a problem hiding this comment.
It is strange to have an exit here. It would look better inside doTrigger.
Note: in case if the user wants to execute some action and there are no available actions, we need to DieNotNil.
|
|
||
| action := args[1] | ||
| if !slices.Contains(allowed, action) { | ||
| subcommands.DieNotNil(fmt.Errorf("Invalid action: %s", action)) |
There was a problem hiding this comment.
Maybe, we also show what are the available actions in this case?
| if len(action) > maxActionLen { // Device Gateway has max len of 48 | ||
| subcommands.DieNotNil(fmt.Errorf("Action name(%s) too long. Max length is %d", action, maxActionLen)) | ||
| } | ||
| id, err := nanoid.New(idLen) |
There was a problem hiding this comment.
Do we really need a NanoID dependency here? Ain't the rand.Text()[:15] good enough.
There was a problem hiding this comment.
I guess not. I just find nano ids to be much more visibly pleasant:
diag_8u_42JiMRZ9VAhR feels better on my eyes than diag_UPBBEVTHF4FQWIZ when I'm trying to look things up.
There was a problem hiding this comment.
I see no difference between these IDs... it's just a copy-paste either way.
My point was to have one less dependency.
|
|
||
| // For the random part anything >= 15 characters will give us about 1,000 | ||
| // years before a collision (assuming 100 IDs per second): | ||
| // https://zelark.github.io/nano-id-cc/ |
There was a problem hiding this comment.
No need for this comment - it is a common wisdom.
2 participants
No description provided.