-
Notifications
You must be signed in to change notification settings - Fork 35
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
✨ Update Issue Description #773
base: main
Are you sure you want to change the base?
Conversation
Issue Description field might be too short to provide enough information about fixing the issue, appending incident Message to it to make it more useful. This is more a workaround/quickfix, long-term solution is explained in ticket below. Related to: https://issues.redhat.com/browse/MTA-4421 and konveyor/kantra#341 Signed-off-by: Marek Aufart <[email protected]>
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
Files: m.Files, | ||
// Append Incident Message to Description to provide more information on Issue detail | ||
// (workaround until Analyzer output Violation struct gets updated to provide better structured data) | ||
Description: fmt.Sprintf("%s\n\n%s", m.Description, m.Message), |
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 wonder if this comment is accurate or necessary. What is the anticipated change in the analyzer data?
This report is a denormalization already.
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 think the main change should be adding Message field to the Violation/Issue, proposal that was discussed mainly with Juanma, Ramon and Shawn is konveyor/analyzer-lsp#452 (reply in thread)
When implementing the proposal, the Violation struct in analyzer-lsp would be changed (added fields and its population) and components using it as dependency including builder in tackle2-addon-analyzer, likely also Hub and UI will need adapt to this struct change too. That update in Hub should remove change introduced in this PR.
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.
After further review, including n.Message
in the result set may greatly change the nature (cardinality) of this report. We also need to consider how pagination will impact this.
Let's consider this further.
(Messages are the same for all Incidents within the Issue) |
Never mind. The Group(by) ensures the desired cardinaltiy. |
Issue Description field might be too short to provide enough information about fixing the issue, appending incident Message to it to make it more useful.
This is more a workaround/quickfix, long-term solution is explained in ticket below and at konveyor/analyzer-lsp#452
Related to: https://issues.redhat.com/browse/MTA-4421 and Hub version of konveyor/kantra#341