Skip to content
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

DOCS-3106: Document CLI usability improvements #3960

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JessamyT
Copy link
Collaborator

@JessamyT JessamyT commented Feb 1, 2025

No description provided.

@viambot viambot added the safe to build This pull request is marked safe to build from a trusted zone label Feb 1, 2025
Copy link

netlify bot commented Feb 1, 2025

Deploy Preview for viam-docs ready!

Name Link
🔨 Latest commit 2da58ba
🔍 Latest deploy log https://app.netlify.com/sites/viam-docs/deploys/67ad21b6c28a5500089727ee
😎 Deploy Preview https://deploy-preview-3960--viam-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 34 (🔴 down 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 92 (no change from production)
PWA: 70 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to address the full scope of the work done here. Part of the scope was to be consistent for flags. That means we should use one flag consistently in the docs (--org possibly for the orgs set) and document the aliases as options.

Copy link
Member

@purplenicole730 purplenicole730 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks good to me, waiting on the changes Naomi asked for before I approve.
I would also ask @stuqdog for a review, since he introduced profiles and made the machine/part flag changes

viam profiles remove --profile-name=<name-of-profile-to-remove>
```

Examples:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples are great for showing how to create and store profiles, but don't highlight how to actually use these profiles. Might be useful to have an example of something like viam --profile=<name-of-profile-to-use> machines list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call--adding!

@@ -1191,45 +1244,42 @@ Examples:
viam machines list

# get machine status
viam machines status --machine=123 --organization="Robot's Org" --location=myLoc
viam machines status --machine=123 --location=myLoc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little unclear about why we don't include organization anymore here, but we include it immediately above? Is the goal for one to be all flags and the other to be only mandatory flags? If so, we can probably get rid of location here as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that was how I was writing this. Oops yes will remove location.

| `--organization` | Organization name or ID that the machine belongs to | `list`, `status`, `logs`, `part` | **Required** |
| `--location` | Location name or ID that the machine belongs to or to list machines in | `list`, `status`, `logs`, `part` | **Required** |
| `--organization` | Organization name or ID that the machine belongs to | `list`, `status`, `logs`, `part` | Optional |
| `--location` | Location name or ID that the machine belongs to or to list machines in | `list`, `status`, `logs`, `part` | Optional |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what command this all is so I may be wrong, but machine below is still listed as Required but we also have part as a flag. I think if part is useful information then machine becomes optional (because machine can often be inferred from part, provided it's given as a part-id).

Copy link
Collaborator Author

@JessamyT JessamyT Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah I guess I can mark machine as optional since they could just use part-id... I'm also realizing we currently list both machine and machine-id in this chart, and show that machine-id is required for api-key.

When I run viam machines api-key create -h currently, it shows machine-id and robot-id as aliases of each other but machine is not an alias for this command. Should I create a ticket for that? I want to combine the machine and machine-id lines in this table but it seems the aliases go one way but not the other for some commands.

For id vs name, I'm tempted to simply tell users to always use the (part/machine/org/loc) ID instead of the name, since IDs are unique even across different orgs (right?) whereas names aren't, and it's simpler to say "use ID" rather than say "use ID or if you use name, then you also need to specify organization and location name, or location ID." But, names can be friendlier than IDs--thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(BTW this table is for the machines command)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think erring on the side of "just use the ID" makes sense! For the API key stuff in particular, currently passing a name isn't supported so I think it makes sense to not include the name aliases here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The machine-id vs machine lines in this table are still weird since api-key still needs machine-id but machine seems to be the preferred flag for status etc. so I think this may be as good as can be for now, and hope people will use the --help flag since it's probably easier to use that than look in the docs when using the CLI anyway.

For the --machine explanation I'm changing it to more strongly recommend ID, and mark it and --part as required for their respective commands.

@JessamyT
Copy link
Collaborator Author

This doesn't seem to address the full scope of the work done here. Part of the scope was to be consistent for flags. That means we should use one flag consistently in the docs (--org possibly for the orgs set) and document the aliases as options.

@npentrel In line with the alias improvement goal to be more opinionated and reduce clutter, I'm thinking it'd be best not to document the aliases here either.

Latest commit fixes the viam machines part args for consistency, but there are other commands e.g. viam module upload takes org-id specifically, and viam data tag ids add and viam dataset data add ids still seem to require e.g. location-id. I think it makes sense to sweep them later once they're all consistent.

@JessamyT JessamyT requested review from npentrel and stuqdog February 11, 2025 17:49
docs/dev/tools/cli.md Outdated Show resolved Hide resolved
| `--organization` | Organization name or ID that the machine belongs to | `list`, `status`, `logs`, `part` | **Required** |
| `--location` | Location name or ID that the machine belongs to or to list machines in | `list`, `status`, `logs`, `part` | **Required** |
| `--organization` | Organization name or ID that the machine belongs to | `list`, `status`, `logs`, `part` | Optional |
| `--location` | Location name or ID that the machine belongs to or to list machines in | `list`, `status`, `logs`, `part` | Optional |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think erring on the side of "just use the ID" makes sense! For the API key stuff in particular, currently passing a name isn't supported so I think it makes sense to not include the name aliases here.

@npentrel
Copy link
Collaborator

45d36b0 was what I was looking for for the organization parameter but the same needs to happen for --location and --location-id and --machine and --machine-id. We should pick one and use it I think. Are you saying all the remaining occurrances are where SDK work hasn't been done yet? Are there tickets for that work that you are following? Have you created a follow up ticket already and are there SDK tickets we can block that on?

I think there's still a part of the work not documented here, namely module generate work. Unless you have another ticket for that? I don't see one.

Copy link
Collaborator

@npentrel npentrel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as there are tickets to follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to build This pull request is marked safe to build from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants