Skip to content

Conversation

metacosm
Copy link
Collaborator

@metacosm metacosm commented Apr 9, 2025

Signed-off-by: Chris Laprun claprun@redhat.com

@metacosm metacosm self-assigned this Apr 9, 2025
@metacosm metacosm requested review from csviri and xstefank April 9, 2025 17:58

@SuppressWarnings("unchecked")
private SSABasedGenericKubernetesResourceMatcher<R> matcher = defaultMatcher;
private SSABasedGenericKubernetesResourceMatcher<R> matcher =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be this rather part of the configuration?

The current approach is that name and class and config are the params and configuration contains everything else. This might go there just as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wanted to keep it simple first but it definitely could go on the config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@metacosm do you to do this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I made it so it is now configured instead of settable manually.

Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
Signed-off-by: Chris Laprun <claprun@redhat.com>
@csviri
Copy link
Collaborator

csviri commented May 8, 2025

Might be nice to see this on some integration test also. (But since it is only a configuration value it is not necessary)

Copy link
Collaborator

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

LGTM

@metacosm metacosm merged commit 5b673a4 into main May 12, 2025
25 checks passed
@metacosm metacosm deleted the config-matcher branch May 12, 2025 07:57
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.

3 participants