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

Convert to IMPORT instead of PACKAGE #103

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

jmle
Copy link
Contributor

@jmle jmle commented Nov 28, 2023

Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
@jmle jmle force-pushed the import-instead-package branch from d92c774 to 95eaa69 Compare November 28, 2023 11:19
Copy link

github-actions bot commented Nov 28, 2023

Overall success rate: 80.25% (1897/2364)

@shawn-hurley
Copy link
Collaborator

We need to make sure that this does cause other issues, like finding matches when it shouldn't.

This was the initial reason to move to the package. IDK did how to do this systematically, but @pranavgaikwad did some great work putting it together.

If this solves this, I will be happy, but did want to make sure that we don't flip-flop issues :)

@jmle
Copy link
Contributor Author

jmle commented Nov 30, 2023

@shawn-hurley PACKAGE seems to work fine when looking for packages exclusively, but we have stuff like this in the ruleset:

    - java.referenced:
        location: PACKAGE
        pattern: java.net.(Socket|MulticastSocket|DatagramSocket|InetSocketAddress)*

which doesn't make sense, since the reference is simply not a package, but a class. IMPORT, on the other hand, seems to work for both packages and classes.

@jmle
Copy link
Contributor Author

jmle commented Nov 30, 2023

@shawn-hurley ...aaand then I just found this rule, which doesn't work with IMPORT but does with PACKAGE...

    - java.referenced:
        location: IMPORT
        pattern: javax.enterprise.deploy*

I'm thinking it could be worth issuing both an IMPORT and PACKAGE search whenever there is one of them, to be able we capture everything

@jmle jmle added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023
… PACKAGE

Signed-off-by: Juan Manuel Leflet Estrada <jleflete@redhat.com>
@jmle jmle removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 1, 2023
@fabianvf fabianvf closed this Jan 2, 2024
@fabianvf fabianvf reopened this Jan 2, 2024
@shawn-hurley
Copy link
Collaborator

I think that, for now, we take this as is; IF we find more and more where it becomes a problem, I think we open a new one and figure out how to do both and then make sure that we can filter out the results that shouldn't match.

@jmle jmle merged commit c158c36 into konveyor:main Jan 3, 2024
6 checks passed
@jmle
Copy link
Contributor Author

jmle commented Jan 3, 2024

I have merged it, although I guess that we'll have to see whether we want to keep using the shim for making changes in the rulesets, or if we want to change the rulesets directly instead.

@fabianvf
Copy link
Collaborator

fabianvf commented Jan 3, 2024

I think if we can we'll want to keep making changes to the shim, since that will also help users with custom rulesets

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.

[BUG] Rule socket-communication-00000 not matching correctly
5 participants