-
Notifications
You must be signed in to change notification settings - Fork 103
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
https://issues.redhat.com/browse/ACM-17505 -- Manage devices #7490
Conversation
b75487a
to
ca90ed1
Compare
* a description of the device hardware and operating system | ||
* an X.509 Certificate Signing Request which includes the cryptographic identity of the device to obtain the initial management certificate | ||
At this point, the device is not considered trusted and remains quarantined in a device lobby until an authorized user approves or denies the request. |
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.
Let's remove any of the extra language like this: At this point
Since this is a task/procedure, we would need a gerund in the title.
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.
Since this is a task/procedure, we would need a gerund in the title.
I've separated them ("Enrollment" vs "Enrolling devices using the CLI") since you commented so now there's a clear distinction even when looking at the titles
0ffa5dc
to
455a64e
Compare
I removed the following content from the PR:
As these are quite big, I'll handle them in a separate PR. |
455a64e
to
c55b180
Compare
edge_manager/edge_mgr_enroll.adoc
Outdated
@@ -0,0 +1,68 @@ | |||
[#enroll] | |||
= Enrolling devices |
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.
Could we merge the Enrollment section from Image building with this section?
c55b180
to
3016a77
Compare
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.
Overall looks great! You may want to consider adding "device" to the file names because the names will probably clash once you add managing/viewing fleets.
edge_manager/edge_mgr_manage.adoc
Outdated
* If your device configurations vary for each device. | ||
* If you need control over the order and timing of updates. |
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.
This is probably taken from outdated docs?
- Devices in a fleet can vary if parameters are used in the fleet template
- The order and timing of updates can be controlled with rollout policies and disruption budgets
I would say that devices might be updated individually if there are few unrelated devices, or perhaps if some external automation will be responsible for updates.
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.
Yes, these are mentioned in the upstream docs: https://github.com/flightctl/flightctl/blob/main/docs/user/managing-fleets.md#understanding-fleets
few unrelated devices
Does that mean that they have different configs?
Anyway, I'll update this part based on your comment! Something like:
Managing your devices individually is advantageous in the following scenarios:
- If a few devices have different configuration.
- If you use external automation for updating the device.
edge_manager/edge_mgr_view.adoc
Outdated
[#view-device-prereqs] | ||
== Prerequisites | ||
|
||
* You must install the {rhem} CLI. |
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.
Also log in ("flightctl login" command)
c8e8f9b
to
7189ac3
Compare
+ | ||
[source,bash] | ||
---- | ||
flightctl get enrollmentrequests |
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.
This command is not accurate, it will list all the existing Enrollment requests in the system, include the already approved ones.
A better command for this could be flightcontrol get er --field-selector="status.approval.approved != true"
@fzdarsky This issue is also present in the upstream documentation.
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.
Thanks @celdrake! However, I see flightctl
vs flightcontrol
. Could you please confirm which command is the right one?
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.
Apologies, I made some typos in the first part of the command.
Yes, it should be flightctl
. Plus I added the abbreviated "er" instead of "enrollmentrequests". I think the longer form is preferred as it's what's used in all other examples.
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.
No worries, thanks for confirming! This is the updated command:
flightctl get enrollmentrequests --field-selector="status.approval.approved != true"
7189ac3
to
4bbdb7b
Compare
4bbdb7b
to
079f7a3
Compare
Complete the following steps: | ||
|
||
. List all devices that are currently waiting for approval by running the following command: | ||
|
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.
In all the steps where they run a command, I would just that they have a command coming up:
Run the following command to.....
Or .... Run the following command:
This helps a user know a command is how they complete the task. Picture a screen reader and a user with limited vision. It can help orient them.
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 get what you're saying but wouldn't phrasing it this way ("do task by running the following command") be better for a screen reader?
Just like skimming through the text while looking for a specific task, with the current phrasing, you'd hear the task first. If I phrase every single task as "Run the following command to do the task:" then the user would have to listen to "Run the following command to..." every single step before knowing what the command is for. Plus the "following command" would be far away from the actual command.
As for this version:
List all devices that are currently waiting for approval. Run the following command:
This doesn't really connect the two sentences with how you complete the task.
I'm happy to change the steps but I wanted to bring up my concerns about it.
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 either and probably a mix of both--to not sound like robots. It would read something like
S_tep 1. List.... period. Run the following command "colon"
new line
then introduce the command._
Vs
_Step 1. List _____ period
new line...
Give user command._
Previously there was only: "List all devices."
I think it is fine either way, I have never gotten any negative feedback on either method, as long as we introduce what's coming up next. The actual task/step is to run a command in order to list. I like: Run the following command to list, but we have a mix.
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
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: amolnar-rh, Samudelacruz77, swopebe The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
e4a6f6a
to
b9a41ed
Compare
New changes are detected. LGTM label has been removed. |
…lling, Viewing, Labeling sections
b9a41ed
to
dcc5501
Compare
After provisioning, you need to enroll your devices to manage them with the {rhem}. | ||
Device management includes organizing, monitoring, and updating your devices with the {rhem}. |
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.
How about we mention that RHEM manages the device lifecycle: enrollment --> management (organizing/updating/monitoring of OS, config, and app workloads) --> decommissioning?
You can manage your devices individually or in a fleet. | ||
Managing your devices individually is advantageous in the following scenarios: | ||
|
||
* If a few devices have different configuration. | ||
* If you use external automation for updating the device. |
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.
The ability to focus on managing a fleet as an aggregate instead of having to manage individual devices is one of the selling points of RHEM.
I'd like to make sure users understand that they can and should be managing devices as fleets, due to its advantages, but that they can manage them individually if they have to. The current text seems to suggest the opposite.
So maybe something like:
"RHEM enables you to manage manage a whole fleet of devices as a single managed object instead of managing many devices individually. This has many advantages like being able to specify desired state only once, no matter the number of managed devices. Fleet managemenet is described in section ....
This section focusses on the management of an individual devices and is the foundation for understanding fleet management, but also for managing devices individually, for example if
- you only have a few devices with vastly different configuration or
- you use external automation for orchestrating device updates.
Issue: https://issues.redhat.com/browse/ACM-17505
To preview, select the file in this folder with the same name as the branch name from this PR.