NO-ISSUE: installing-pending-user-action timeout docs update#10252
NO-ISSUE: installing-pending-user-action timeout docs update#10252carbonin wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@carbonin: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughDocumentation updates add timeout/error outcome to the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/installing-pending-user-action-timeout-behavior.md (2)
31-34: Add language identifier to fenced code block.For better markdown compliance, consider adding a language identifier to the fenced code block. Since this is a user-facing message rather than code, you can use
text:📝 Suggested improvement
-``` +```text Expected the host to boot from disk, but it booted the installation image - please reboot and fix boot order to boot from disk [Model] [Serial] (disk-name, disk-id)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/installing-pending-user-action-timeout-behavior.mdaround lines 31 -
34, Add the language identifier "text" to the fenced code block that contains
the user-facing message "Expected the host to boot from disk, but it booted the
installation image - please reboot and fix boot order to boot from disk [Model]
[Serial] (disk-name, disk-id)" so the markdown fence becomestext ...to
improve markdown compliance and rendering; locate the fenced block in
docs/installing-pending-user-action-timeout-behavior.md and update the opening
fence accordingly.</details> --- `66-71`: **Add language identifier to fenced code block.** For better markdown compliance, consider adding a language identifier to the fenced code block. Since this is a user-facing message, you can use `text`: <details> <summary>📝 Suggested improvement</summary> ```diff -``` +```text Host timed out when pulling the configuration files. Verify in the host console that the host boots from the OpenShift installation disk (disk-name, disk-id) and has network access to the cluster API. The installation will resume after the host successfully boots and can access the cluster API ``` ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/installing-pending-user-action-timeout-behavior.mdaround lines 66 -
71, Add a language identifier to the fenced code block containing the Host timed
out message by changing the opening triple backticks totext so the block begins withtext and ends with ```; this ensures proper markdown highlighting
and compliance for the multi-line message (the fenced code block with the host
timeout instructions).</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@docs/installing-pending-user-action-timeout-behavior.md:
- Around line 254-257: Update the documentation references to point at the
correct locations and function names: replace the incorrect cluster validation
reference toenoughMastersAndWorkerswithHasEnoughMastersAndWorkersand
update its path tointernal/common/common.go:939, and change the host counting
reference forHostsInStatustointernal/common/common.go:916; ensure lines
254-257 mentionHostsInStatusat internal/common/common.go:916 and
HasEnoughMastersAndWorkersat internal/common/common.go:939 so the doc links
match the codebase.- Line 63: The docs reference only part of the reboot timeout transition; update
the code reference range in the documentation so it covers the entire
TransitionRule for the reboot timeout (including its closing brace) rather than
stopping before it—locate the TransitionRule named/related to the reboot timeout
in the statemachine and extend the documented line range so the full rule is
included.
Nitpick comments:
In@docs/installing-pending-user-action-timeout-behavior.md:
- Around line 31-34: Add the language identifier "text" to the fenced code block
that contains the user-facing message "Expected the host to boot from disk, but
it booted the installation image - please reboot and fix boot order to boot from
disk [Model] [Serial] (disk-name, disk-id)" so the markdown fence becomes
text ...to improve markdown compliance and rendering; locate the fenced
block in docs/installing-pending-user-action-timeout-behavior.md and update the
opening fence accordingly.- Around line 66-71: Add a language identifier to the fenced code block
containing the Host timed out message by changing the opening triple backticks
totext so the block begins withtext and ends with ```; this ensures
proper markdown highlighting and compliance for the multi-line message (the
fenced code block with the host timeout instructions).</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: openshift/coderabbit/.coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `56490c98-0f24-4cca-8785-6c9ce5d59163` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c9164d448f971c1cc3c9aad3cee04c9b833f6cab and ad5ad5044db55f800a5c9c14448fe856fae803b1. </details> <details> <summary>⛔ Files ignored due to path filters (2)</summary> * `docs/HostStatus.dot` is excluded by `!**/*.dot` * `docs/HostStatus.png` is excluded by `!**/*.png` </details> <details> <summary>📒 Files selected for processing (2)</summary> * `docs/architecture.md` * `docs/installing-pending-user-action-timeout-behavior.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| **Code References:** | ||
| - Host timeout transition: `internal/host/statemachine.go` (HasPendingUserActionTimedOut condition) | ||
| - Cluster validation: `internal/cluster/transition.go:517-554` (enoughMastersAndWorkers function) | ||
| - Host counting: `internal/cluster/common.go:192-206` (HostsInStatus function) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the locations of viability check functions
# Search for enoughMastersAndWorkers function
echo "=== Searching for enoughMastersAndWorkers ==="
rg -n 'func.*enoughMastersAndWorkers' internal/
# Search for HostsInStatus function
echo "=== Searching for HostsInStatus ==="
rg -n 'func.*HostsInStatus' internal/
# Search for HasEnoughMastersAndWorkers function
echo "=== Searching for HasEnoughMastersAndWorkers ==="
rg -n 'func.*HasEnoughMastersAndWorkers' internal/Repository: openshift/assisted-service
Length of output: 416
🏁 Script executed:
# Check if enoughMastersAndWorkers exists in transition.go
echo "=== Checking transition.go for enoughMastersAndWorkers ==="
rg -A 5 'enoughMastersAndWorkers' internal/cluster/transition.go
# Check what's actually at lines 517-554 in transition.go
echo "=== Lines 517-554 in transition.go ==="
sed -n '517,554p' internal/cluster/transition.go | cat -n
# Check the context around HostsInStatus in common.go
echo "=== Lines 910-925 in common.go (around HostsInStatus) ==="
sed -n '910,925p' internal/common/common.go | cat -n
# Verify if transition.go calls HasEnoughMastersAndWorkers or similar
echo "=== Searching transition.go for HasEnoughMastersAndWorkers usage ==="
rg -B 2 -A 2 'HasEnoughMastersAndWorkers' internal/cluster/transition.goRepository: openshift/assisted-service
Length of output: 3041
Correct the code reference paths for cluster viability checks.
The documentation references incorrect file paths and function names:
enoughMastersAndWorkersfunction does not exist; should referenceHasEnoughMastersAndWorkersHostsInStatusis located atinternal/common/common.go:916, notinternal/cluster/common.go:192-206HasEnoughMastersAndWorkersis located atinternal/common/common.go:939, notinternal/cluster/transition.go:517-554
Update lines 254-257 to reference:
- Host counting:
internal/common/common.go:916(HostsInStatus function) - Cluster validation:
internal/common/common.go:939(HasEnoughMastersAndWorkers function)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/installing-pending-user-action-timeout-behavior.md` around lines 254 -
257, Update the documentation references to point at the correct locations and
function names: replace the incorrect cluster validation reference to
`enoughMastersAndWorkers` with `HasEnoughMastersAndWorkers` and update its path
to `internal/common/common.go:939`, and change the host counting reference for
`HostsInStatus` to `internal/common/common.go:916`; ensure lines 254-257 mention
`HostsInStatus` at internal/common/common.go:916 and
`HasEnoughMastersAndWorkers` at internal/common/common.go:939 so the doc links
match the codebase.
There was a problem hiding this comment.
Rabbit has a point, the function moved :)
Code references in general are very hard to maintain, I would discourage their use, unless we find some method that would avoid the staleness issue
There was a problem hiding this comment.
@rccrdpccl, totally agree — line-number references in docs are notoriously fragile. A few approaches that can reduce (though not eliminate) the staleness problem:
-
GitHub permalink to a specific commit SHA — Instead of
internal/foo/bar.go:123, link tohttps://github.com/openshift/assisted-service/blob/<SHA>/internal/foo/bar.go#L123. The line stays frozen at that commit, so it's never silently wrong — though it does go stale in the sense that it won't track future refactors. -
Reference symbols/function names only, no line numbers — Prose like "see
HasEnoughMastersAndWorkersininternal/common/common.go" is far more resilient than line numbers because symbol renames are usually grep-able, whereas line shifts are invisible. -
Automated link-rot check — A CI step (e.g., a small script) that greps for every symbol name mentioned in the doc and fails if any are missing from the codebase. This catches renames at PR time rather than months later.
For this PR, given the docs-only nature, the simplest fix would be to drop line numbers entirely and just reference function/file names. That way the references remain useful without the high maintenance cost.
📝 Learnings were identified and not saved (knowledge base disabled). Enable
|
@carbonin: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Document the timeout behavior in
installing-pending-user-actionFollowup to #10202
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit