Skip to content
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

✨ dismissal of incidents #197

Merged

Conversation

shawn-hurley
Copy link
Contributor

Tried to capture the changes across the projects, keeping the logic for dismissal in the analyzer and adding the UI/UX for how a user could use this feature

@shawn-hurley shawn-hurley changed the title ✨ adding initial pass at solving the dismissal of incidents ✨ dismissal of incidents Aug 14, 2024
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of things:

  1. I'm genuinely curious how much value we would "lose" by simply allowing users to dismiss at the rule level. This may be a declaration of ignorance but it isn't immediately obvious scenarios where a particular rule generates issues that must be fixed at the same time as ones that don't need to be addressed.
  2. I wonder if we could perform the dismissing/filtering as a post-analysis step. My argument for this perspective is that I view this concern as a purely presentation layer concern. That is, at the analyzer level, these are absolutely still incidents; they just happen to be ones we don't want to see in analysis reports in the Konveyor UI or static-report.

There are some implications to these thoughts that I don't want to ramble on if I'm heading in the totally wrong direction.

enhancements/dismiss-issues/README.md Outdated Show resolved Hide resolved
enhancements/dismiss-issues/README.md Outdated Show resolved Hide resolved
enhancements/dismiss-issues/README.md Outdated Show resolved Hide resolved
@shawn-hurley
Copy link
Contributor Author

I'm genuinely curious how much value we would "lose" by simply allowing users to dismiss at the rule level. This may be a declaration of ignorance but it isn't immediately obvious scenarios where a particular rule generates issues that must be fixed at the same time as ones that don't need to be addressed.

You know, this is a good question that I don't know if I have an answer too.

One thing that I was thinking of is work at the lowest level to give users the most flexability, and we could add a "bulk" update based on ruleID as a follow up. Now this doesn't mean that it is correct by any stretch of the imagination.

Something that I was thinking about was a use case like local-storage. They are mandatory issues, that tell you to fix something, but let's say you have two use cases for local storage. In one use case, you are using a local file for configuration, this would most likely be a configmap in k8s and still use local storage. But let's say that you are also using a cache of data over local storage, this probably should be solved.

So for the tackle test app case, you probably across the entire portfolio want to disable that particular incident for the application config, because you plan to solve that with a configmap across the entire portfolio.

Just a thought though, wondering if that even makes sense.

@shawn-hurley
Copy link
Contributor Author

I wonder if we could perform the dismissing/filtering as a post-analysis step. My argument for this perspective is that I view this concern as a purely presentation layer concern. That is, at the analyzer level, these are absolutely still incidents; they just happen to be ones we don't want to see in analysis reports in the Konveyor UI or static-report.

I agree they are still incidents, but because we have two layers (and potentially more with the specific language builds) doing this, I figured two things:

  1. That adding it into the analyzer means a single implementation that everyone gets to use.
  2. Making it a first class citizen (like we did with insights for instance) gives the output of the analyzer more structure to get what you really want. Think about the Kai case, where they really just use the output of the analyzer. If in a company they want to ignore certain specific incidents (say similar to the local storage outlined above) then you probably want this to live in the analyzer.

The analyzer could do this as a post process OR it could just add a field on the incidents instead of making a whole new list. I leaned to the whole new list because it "feels" like something you would want to see at the top level. I could be convinced that it should be at the lower level and just be a field on the incident, but I can't think of a compelling reason at the moment.

Does that make sense? Should I add any of this to the enhancement?

enhancements/dismiss-issues/README.md Outdated Show resolved Hide resolved
enhancements/dismiss-issues/README.md Outdated Show resolved Hide resolved

One goal should be that a user, once they make these dismissals, they will not need to do this on each subsequent run. To achieve this, we should have a new subcommand that will manage the DIF for a given application.

```kantra dismissed-incidents
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this make more sense as a subcommand for analyze? It is only relevant to the analyze command, and that way it wouldn't be on the same level as kantra transform and kantra version etc..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering how important that is, if you feel strongly I can change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No strong feelings

@dymurray dymurray added this to the v0.6.0 milestone Aug 20, 2024
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jortel jortel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@shawn-hurley shawn-hurley merged commit a85b2e6 into konveyor:master Aug 26, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants