-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat: improve Quarkus component detection #21
Conversation
9963e0d
to
f05732a
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
==========================================
+ Coverage 67.89% 67.96% +0.07%
==========================================
Files 11 11
Lines 1523 1539 +16
==========================================
+ Hits 1034 1046 +12
- Misses 423 426 +3
- Partials 66 67 +1 ☔ View full report in Codecov by Sentry. |
Is there anything else I need to do here? |
@@ -51,11 +51,14 @@ func (j JavaEnricher) DoEnrichLanguage(language *model.Language, files *[]string | |||
if gradle != "" { | |||
language.Tools = []string{"Gradle"} | |||
detectJavaFrameworks(language, gradle) | |||
language.FrameworkPreferred = true |
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.
Shall we add this as part of the pkg/utils/langfiles/resources/languages-customization.yaml
like:
Java:
configuration_files:
- "pom.xml"
- "build.gradle"
component: true
framework_preferred: true
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'm temporarily request changes, as it is still under consideration regarding the changes we make in the component detection behavior for java components.
More detailed, I want to confirm team-wise that we would like to introduce this new behavior inside the component_recognizer.go
Resuming discussion for the PR
//note that we don't specify the group id because this can sometimes be a property substitution
if hasFwk, _ := hasFramework(config, "", "quarkus-maven-plugin"); hasFwk {
language.Frameworks = append(language.Frameworks, "Quarkus")
} else if hasFwk, _ = hasFramework(config, "", "io.quarkus.gradle.plugin"); hasFwk {
language.Frameworks = append(language.Frameworks, "Quarkus")
}
That said, IMO I would suggest that we could keep the fix for quarkus |
Sorry, I completely missed the review on this. The reason why I did the filtering where it is was because I wanted to consider the whole project when making this change (i.e. the hasFrameworks field). If no frameworks are detected it keeps the existing behavior. |
Quarkus projects often have lots of modules that are not intended to be components, but still have io.quarkus dependencies. This commit will only mark a module as a Quarkus component if it explicitly references the Quarkus plugin that creates a runnable application. In addition if a Java project contains components that explicitly specify frameworks then modules without frameworks will be removed. This will help with the usability issues identified in RHTAPBUGS-552 Signed-off-by: Stuart Douglas <stuart.w.douglas@gmail.com>
f05732a
to
ce15ea7
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mike-hoang, stuartwdouglas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@stuartwdouglas no worries! I have went again through the proposed workaround. As mentioned in a previous comment, atm we are not covering all the frameworks so we may have cases that we are excluding a legit component. I tried to run an example for a hypothetical monolithic repo which has different components inside. Example dir
From the four above components the 3 of them will be enriched with a framework (e.g. openliberty, quarkus and react). There is one which will not be enriched with a framework, the JSF (randomly picked as example). With quarkus improvements we will be able to detect only 3 components, while with the existing functionality we are able to detect all 4 components stored in root dir. |
I guess to make this decision properly you would need to track dependency information between the modules. If a module with no framework is a dependency of another module with a framework, then this module is likely not a component. If a module is standalone then it is likely a component even without a detected framework. That said I am not really sure how to make such a change in the current code base, I think it would be a fairly significant change. |
@stuartwdouglas mostly I agree with what you are saying. The only thing I'd like to note, is that IMO a module that has no framework and is a dependency of another module could be a component. It could be detected, but we could also add a field in alizer's response mentioning any related/dependent components for each component. I see this as a cool feature for alizer. We could also add a flag like "parent-components-only" (name not important) so a user can choose if they want to see only parent components (makes this workaround backwards compatible too). WDYT? |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 60 days. |
This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 60 days. |
This PR is stale because it has been open for 90 days with no activity. Remove stale label or comment or this will be closed in 60 days. |
Quarkus projects often have lots of modules that are not intended to be components, but still have io.quarkus dependencies.
This commit will only mark a module as a Quarkus component if it explicitly references the Quarkus plugin that creates a runnable application.
In addition if a Java project contains components that explicitly specify frameworks then modules without frameworks will be removed.
This will help with the usability issues identified in RHTAPBUGS-552.
Testing against Apicurio Registry this reduces the component count down from 50 to 8, which is expected.