OTA-1174: upgrade status: --details shows descriptions and references - #1690
OTA-1174: upgrade status: --details shows descriptions and references#1690petr-muller wants to merge 10 commits into
upgrade status: --details shows descriptions and references#1690Conversation
|
@petr-muller: This pull request references OTA-1174 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: petr-muller 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
|
/uncc @ardaguclu @soltysh |
DavidHurta
left a comment
DavidHurta
left a comment
There was a problem hiding this comment.
A well-written code. I even had a good chuckle reading the tests.
I have left some notes, mainly nitpicks and suggestions for small changes. I have not found any bugs.
Note: Tested locally on the submitted test data.
| level: errorImpactLevel, | ||
| impactType: apiAvailabilityImpactType, | ||
| summary: `Cluster Operator testOperator is unavailable (Broken)`, | ||
| description: "Operator is broken\nand message has linebreaks", |
There was a problem hiding this comment.
I would suggest to update the test name to reflect the change.
Something like: "insights flatten linebreaks in messages" -> "insights do not flatten linebreaks in messages"
| t.Run(cv, func(t *testing.T) { | ||
| t.Parallel() | ||
| co := strings.Replace(cv, "-cv.yaml", "-co.yaml", 1) | ||
| for _, variant := range variants { |
There was a problem hiding this comment.
We may want to update the README file to reflect that two output files are now required.
| detailedOutput(w, displayData) | ||
| } else { | ||
| tabulatedOutput(w, displayData) | ||
|
|
There was a problem hiding this comment.
Nitpick: A redundant line.
| func (i updateInsight) incomplete() bool { | ||
| return i.startedAt.IsZero() || i.impact.incomplete() || i.remediation.incomplete() | ||
| } |
There was a problem hiding this comment.
At the moment, I agree with this definition of an incomplete update insight for the current code. However, in the future, some of the information may not be easily accessible. For example, in #1689, the startedAt may not be present for some insights. Or the reference link may not be easily obtainable. I am perfectly fine with merging this code at the moment, as it is perfectly correct for the current code and this PR's code, but we'll probably need to address this logic in the future for specific cases.
| } | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nitpick: A redundant line.
|
Thanks for the feedback, will address. I will hold this PR so that #1689 does not accidentally need to rebase & resolve conflicts. /hold |
|
The PR looks good to me. Once @Davoska 's comments are addressed we can merge this. |
openshift-merge-robot
added
the
needs-rebase
3fed3ab to
804d281
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/hold Adapted to #1689 but I'd like to address some quirks |
openshift-ci
Bot
added
the
do-not-merge/hold
With --details, every insight will have two more fields - description and remediation URL (runbook, documentation...). Output is not a table, but a `oc describe`-like tree output.
Move the condition message from the insight summary to its description (which is only shown with --details). Remediation links point to o/runbooks runbook for the matching alert.
Condition messages are propagated via an insight description, which is only shown with `--details` in a tree-like output. This allows us to keep linebreaks originally present in the message, we just need to pad each line enough to align it with the indentation of the message start.
Incomplete insights are not not actionable enough to show them to the user, so we filter them out while processing.
804d281 to
2f3add0
Compare
2f3add0 to
6da69b9
Compare
|
@petr-muller: The following test failed, say
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/test-infra repository. I understand the commands that are listed here. |
|
I'll need to break this one down :( |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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/test-infra repository. |
|
@petr-muller: Closed this PR. |
5 participants
With --details, every insight will have two more fields - description and remediation URL (runbook, documentation...). Output is not a table, but a
oc describe-like tree output. Move the condition message from the insight summary to its description (which is only shown with --details). Remediation links point to o/runbooks runbook for the matching alert. Condition messages are propagated via an insight description, which is only shown with--detailsin a tree-like output. This allows us to keep linebreaks originally present in the message, we just need to pad each line enough to align it with the indentation of the message start. Incomplete insights are not not actionable enough to show them to the user, so we filter them out while processing.