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

Add rule for SourceHttpMessageConverter issue #179

Merged
merged 2 commits into from
Nov 29, 2024

Conversation

jmle
Copy link
Contributor

@jmle jmle commented Nov 29, 2024

See #156

Supersedes #176

Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
Copy link

@rromannissen rromannissen left a comment

Choose a reason for hiding this comment

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

LGTM! Great job, this one was super tricky!

@jmle jmle merged commit e2115bb into konveyor:main Nov 29, 2024
6 checks passed
@rromannissen
Copy link

rromannissen commented Nov 29, 2024

I think we might have overlooked something here. Even if you follow the guidelines in the snippet and change the configureMessageConverters method to add the SourceHttpMessageConverter to the HttpMessageConverter list, the rule would still be still finding incidents in the same locations regardless of the change.

That means that we need to add an and not condition that contains a pointer to the right configuration (again, adding a SourceHttpMessageConverter to the HttpMessageConverter list inside of an invocation to the configureMessageConverters method inside of a class that implements the WebMvcConfigurer interface).

This feels like a pattern for issues that get solved by a change in configuration, always add an and not condition expressing the right configuration to get the issue solved. @jwmatthews @pranavgaikwad @shawn-hurley @fabianvf I wonder if we could leverage this in Kai to inform the LLM of how to fix issues when the solution is not local to where the incidents were found.

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.

2 participants