-
Notifications
You must be signed in to change notification settings - Fork 48
Fix error in CLI machine API key creation doc #4392
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
✅ Deploy Preview for viam-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
@JessamyT ran this by john, inspired by chat with ethan, but your attention would be helpful since you last touched this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think we don't want to document "or name" since if you do that, then you must supply location ID or location name + org (id? name?). Or at least that's what we decided last time this came up, when we removed "or name."
Related: https://viam.atlassian.net/browse/RSDK-9183
#3960 (comment)
Oh and I should say: IIRC I didn't want to lump machine-id and machine into one line in the table because machine-id is specifically required for the one command and not for others, so it'd be hard to combine. And therefore, in some ways clearer to use machine
in the examples where either works.
@JessamyT removed 'or name' from all situations where the command doesn't already include the location and org ids. The table description already mentions name, though (mentioning what you just said about specifying location/org id), so for a small subset of these I feel it's appropriate to leave the 'or name' in place. |
docs/dev/tools/cli.md
Outdated
@@ -691,7 +691,7 @@ viam machines list | |||
viam machines status --machine=123 | |||
|
|||
# create an api key for a machine | |||
viam machines api-key create --machine=123 --name=MyKey | |||
viam machines api-key create --machine-id=123 --org-id=456 --name=MyKey |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you use machine-id you don't need org-id too though do you? since machine-id is unique?
docs/dev/tools/cli.md
Outdated
viam machines logs --organization=<org id> --location=<location id> --machine=<machine id> [...named args] | ||
viam machines api-key create --machine=<machine id> [...named args] | ||
viam machines part list --machine=<machine-id> | ||
viam machines status --organization=<org id> --location=<location id> --machine=<machine id or name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for these templates it almost makes more sense to remove location and org, and only show machine id, since you don't need all three at once, but we've hitherto said that the templates should show all flags even if you wouldn't use them together. (And we say examples need not be so exhaustive.) What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. I'll keep an eye out to see if this causes any problems, but I think it would be much more user-friendly to document the basics, instead of unnecessary arguments.
@JessamyT what do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits, assuming you're confident that these things do indeed all run w/o location and org id
@@ -773,7 +773,7 @@ viam machine part cp --part=123 -r -p machine:my_dir machine:my_file ~/some/exis | |||
| Argument | Description | Applicable commands | Required? | | |||
| -------- | ----------- | ------------------- | --------- | | |||
| `--part` | Part ID for which the command is being issued. | `part` | **Required** | | |||
| `--machine` | Machine ID for which the command is being issued. If machine name is used instead of ID, `--org` and `--location` are required. | `status`, `logs` | **Required** | | |||
| `--machine` | Machine ID or name for which the command is being issued. If machine name is used instead of ID, `--organization` and `--location` are required. | `status`, `logs` | **Required** | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do call it --org in the line below, but i realize we don't in the examples. Could dig back through the old discussion about this one too but don't remember off hand...maybe should change line 778 to match this though?
Co-authored-by: Jessamy Taylor <[email protected]>
🔎💬 Inkeep AI search and chat service is syncing content for source 'Viam Docs' |
Fixes #4390
Also clarify that the
--machine
flag can accept a machine ID or name. Not mentioned: ability to pass--machine-id
or--machine-name
flags instead of--machine
(too pedantic; don't document)