-
Notifications
You must be signed in to change notification settings - Fork 107
Rework Lightspeed docs #4524
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
Rework Lightspeed docs #4524
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideReworks the Lightspeed/Insights documentation by splitting the previously combined host monitoring assembly into separate cloud-focused and project-focused docs, renaming modules and procedures to emphasize hosts terminology, and adding new guidance around Insights analytics, project/cloud configuration, data control, and the new host_registration_insights_inventory parameter while removing superseded content. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jeremylenz
left a comment
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 @Lennonka
Some inline comments
guides/common/modules/con_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_monitoring-hosts-by-using-insights-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_monitoring-hosts-by-using-insights-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_deploying-insights-client-on-registered-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_enabling-insights-analytics-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_excluding-hosts-from-insights-reporting.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_excluding-hosts-from-insights-reporting.adoc
Outdated
Show resolved
Hide resolved
jeremylenz
left a comment
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.
A few more I found while reading the preview
guides/common/modules/proc_deploying-insights-client-on-registered-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_creating-insights-remediation-plan-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_adding-project-server-ssh-key-to-authorized-keys.adoc
Outdated
Show resolved
Hide resolved
|
Relevant preview links: I'm not sure why the preview build produces so many links. |
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.
Hey there - I've reviewed your changes - here's some feedback:
- Since several modules and assemblies were renamed or split (for example,
systems→hostsand the new cloud vs project assemblies), double-check that all cross-references and includes indoc-Managing_Hosts/master.adocand related guides point to the new file names and IDs to avoid broken links. - There is now some conceptual overlap between the cloud and project monitoring assemblies; consider factoring shared explanations (such as what Insights data is collected and how Lightspeed fits in) into a single reusable conceptual module to avoid divergence over time.
- The terminology around Lightspeed vs Insights (and RH Cloud vs project) is central to this restructure; verify that the wording is consistent across the new modules (for example, consistently using ‘hosts’ and a single term for the inventory parameter) so users don’t have to infer that different phrases refer to the same feature.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since several modules and assemblies were renamed or split (for example, `systems` → `hosts` and the new cloud vs project assemblies), double-check that all cross-references and includes in `doc-Managing_Hosts/master.adoc` and related guides point to the new file names and IDs to avoid broken links.
- There is now some conceptual overlap between the cloud and project monitoring assemblies; consider factoring shared explanations (such as what Insights data is collected and how Lightspeed fits in) into a single reusable conceptual module to avoid divergence over time.
- The terminology around Lightspeed vs Insights (and RH Cloud vs project) is central to this restructure; verify that the wording is consistent across the new modules (for example, consistently using ‘hosts’ and a single term for the inventory parameter) so users don’t have to infer that different phrases refer to the same feature.
## Individual Comments
### Comment 1
<location> `guides/common/modules/proc_adding-project-server-ssh-key-to-authorized-keys.adoc:19` </location>
<code_context>
-To remove existing data of a system, navigate to *Inventory* > *Systems* in the {RHCloud}, select the system, and click *Delete*.
-* You have selected an organization and location.
-
-.Procedure
-. In the {ProjectWebUI}, navigate to *{Insights}* > *Inventory Upload*.
-. Select the *Minimal data collection* setting from the dropdown menu under *Settings*.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This SSH key configuration procedure does not provide a verification step to ensure remote execution from {ProjectServer} will work.
Given that the goal is to enable remote execution on {ProjectServer}, it would be helpful to add a verification section describing how to confirm that the key setup works (for example, testing SSH from the `foreman-proxy` user to root on {ProjectServer} or running a simple remote execution job targeting {ProjectServer}).
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/modules/proc*.adoc`
**Instructions:**
Verification steps are included where appropriate.
</details>
</issue_to_address>
### Comment 2
<location> `guides/common/modules/proc_enabling-insights-analytics-for-hosts.adoc:31` </location>
<code_context>
-To remove existing data of a system, navigate to *Inventory* > *Systems* in the {RHCloud}, select the system, and click *Delete*.
-* You have selected an organization and location.
-
-.Procedure
-. In the {ProjectWebUI}, navigate to *{Insights}* > *Inventory Upload*.
-. Select the *Minimal data collection* setting from the dropdown menu under *Settings*.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The procedure for enabling {Insights} analytics and related parameters does not include guidance on verifying that analytics and reporting are actually enabled for the selected hosts.
Because this procedure controls analytics enablement and inventory uploads, it should have a verification section—for example, how to check that the parameters are in effect on a host, that the Insights client runs as expected, and, where applicable, that hosts appear in the {RHCloud} inventory/analytics views.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/modules/proc*.adoc`
**Instructions:**
Verification steps are included where appropriate.
</details>
</issue_to_address>
### Comment 3
<location> `guides/common/modules/ref_exclusion-of-hosts-from-insights-analytics.adoc:21` </location>
<code_context>
+
+ifdef::insights-in-cloud[]
+`host_registration_insights_inventory`::
+Setting this parameter to `false` will exclude the host from the {Project} host inventory that is uploaded to the {RHCloud}.
++
+If you exclude a host that is already reported on the https://console.redhat.com/[{RHCloud}], it will be still removed automatically from the inventory.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase "that is uploaded to the {RHCloud}" uses passive voice and can be rephrased to active voice.
Consider rephrasing to active voice, for example: "from the {Project} host inventory that {Project} uploads to the {RHCloud}" or "from the {Project} host inventory uploaded to the {RHCloud}."
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/*.adoc,guides/common/modules/*.adoc`
**Instructions:**
Avoid passive voice in verbs.
</details>
</issue_to_address>
### Comment 4
<location> `guides/common/modules/ref_exclusion-of-hosts-from-insights-analytics.adoc:23` </location>
<code_context>
+`host_registration_insights_inventory`::
+Setting this parameter to `false` will exclude the host from the {Project} host inventory that is uploaded to the {RHCloud}.
++
+If you exclude a host that is already reported on the https://console.redhat.com/[{RHCloud}], it will be still removed automatically from the inventory.
+However, this process can take some time to complete.
+endif::[]
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The clause "it will be still removed automatically" is in passive voice and should be rewritten in active voice.
You could rephrase this to something like: "{Project} still removes the host automatically from the inventory" or "the host still gets removed automatically from the inventory" to avoid passive voice.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/*.adoc,guides/common/modules/*.adoc`
**Instructions:**
Avoid passive voice in verbs.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ifdef::insights-in-cloud[] | ||
| `host_registration_insights_inventory`:: | ||
| Setting this parameter to `false` will exclude the host from the {Project} host inventory that is uploaded to the {RHCloud}. |
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.
suggestion (review_instructions): The phrase "that is uploaded to the {RHCloud}" uses passive voice and can be rephrased to active voice.
Consider rephrasing to active voice, for example: "from the {Project} host inventory that {Project} uploads to the {RHCloud}" or "from the {Project} host inventory uploaded to the {RHCloud}."
Review instructions:
Path patterns: guides/common/*.adoc,guides/common/modules/*.adoc
Instructions:
Avoid passive voice in verbs.
| `host_registration_insights_inventory`:: | ||
| Setting this parameter to `false` will exclude the host from the {Project} host inventory that is uploaded to the {RHCloud}. | ||
| + | ||
| If you exclude a host that is already reported on the https://console.redhat.com/[{RHCloud}], it will be still removed automatically from the inventory. |
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.
suggestion (review_instructions): The clause "it will be still removed automatically" is in passive voice and should be rewritten in active voice.
You could rephrase this to something like: "{Project} still removes the host automatically from the inventory" or "the host still gets removed automatically from the inventory" to avoid passive voice.
Review instructions:
Path patterns: guides/common/*.adoc,guides/common/modules/*.adoc
Instructions:
Avoid passive voice in verbs.
|
I've vamped it up a bit. This is good enough for now or I could improve it indefinitely! @jeremylenz Can I please get a formal tech ack on these changes? Please, also check if the order of the sections in those chapters makes sense. Using preview is recommended. |
jeremylenz
left a comment
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.
Ordering of the preview looks good to me and all comments are addressed. One optional note below, but consider this a dev ACK from my side. 👍
guides/common/modules/con_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/con_monitoring-hosts-by-using-insights-in-cloud.adoc
Show resolved
Hide resolved
guides/common/modules/con_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
| * If you have problems connecting to {Insights}, ensure that your certificates are up-to-date. | ||
| Refresh your subscription manifest to update your certificates. |
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 do not understand which certificates this refers to: Red Hat manifest on Foreman+Katello (aka https://docs.theforeman.org/nightly/Managing_Content/index-katello.html#Updating_and_Refreshing_Red_Hat_Subscription_Manifests_content-management)? certificates on clients (aka subscription-manager refresh)?
@jeremylenz Could you please weigh in?
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.
Refreshing a manifest updates the upstream consumer identity certificate in the Candlepin owner, which is used by foreman_rh_cloud as credentials to communicate with console dot.
guides/common/modules/con_monitoring-hosts-by-using-insights-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_enabling-insights-analytics-for-hosts.adoc
Outdated
Show resolved
Hide resolved
guides/common/assembly_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
guides/common/assembly_monitoring-hosts-by-using-insights-in-cloud.adoc
Outdated
Show resolved
Hide resolved
guides/common/assembly_monitoring-hosts-by-using-insights-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/assembly_monitoring-hosts-by-using-insights-in-project.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_removing-hosts-from-insights-inventory.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_synchronizing-insights-recommendations-for-hosts.adoc
Outdated
Show resolved
Hide resolved
Co-authored-by: Maximilian Kolb <[email protected]>
|
Anything else? 👀 |
maximiliankolb
left a comment
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.
style-wise LGTM. #4524 (comment) is still unclear to me but this could also be a follow-up.
What changes are you introducing?
Splitting the Monitoring hosts by using Insights (=Lightspeed) chapter
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
host_registration_insights_inventoryAnything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
Contributor checklists
Please cherry-pick my commits into:
Summary by Sourcery
Restructure and split the Lightspeed/Insights host monitoring documentation into separate cloud and project-focused assemblies and modules.
Documentation:
Summary by Sourcery
Restructure the Lightspeed/Insights host monitoring documentation by splitting cloud-based and project-based workflows into separate assemblies and updating related terminology.
Documentation: