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

Adding two new static file extension folders for Java Specific Workflows #97

Merged
merged 4 commits into from
Aug 13, 2024

Conversation

shawn-hurley
Copy link
Contributor

What does this PR do?

When using a spring boot application in Java, many of the static files will be placed in META-INF/resources. Right now these are treated like first class concepts, eventually moving the weight of Java application down far enough to not be selected.

You can see more info about this here: https://spring.io/blog/2013/12/19/serving-static-web-content-with-spring-boot

When using a spring boot application in Java, many of the static files
will be placed in META-INF/resources. Right now these are treated like
first class concepts, eventually moving the weight of Java application
down far enough to not be selected.

Signed-off-by: Shawn Hurley <[email protected]>
@michael-valdron michael-valdron requested review from thepetk and removed request for elsony August 9, 2024 21:26
Copy link
Member

@michael-valdron michael-valdron left a comment

Choose a reason for hiding this comment

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

Changes here looks good to me.

@thepetk I wonder if we should investigate tying project specific static file extension folders with the detection of the project source since there is many specific projects that Alizer could work with, i.e. if it detects it has Java source under the project at least, such as after running languagesFile.GetLanguagesByExtension(extension), include this extra collection of static file extension folders. WDYT?

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.40%. Comparing base (42574ef) to head (4ba6010).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #97      +/-   ##
==========================================
+ Coverage   73.16%   73.40%   +0.24%     
==========================================
  Files          11       11              
  Lines        1565     1568       +3     
==========================================
+ Hits         1145     1151       +6     
+ Misses        351      349       -2     
+ Partials       69       68       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@thepetk thepetk left a comment

Choose a reason for hiding this comment

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

In general the addition of those 2 extra static dirs make sense to me.

Before approving I'd suggest adding an extra test case in the language_recognizer_test.go to make sure for every new feature this issue won't show up again.

@michael-valdron re: to your comment. I'm supportive of creating a spike issue to investigate an improved approach for the analyze command.

My two cents:

  • The analyze command tries to quickly summarize all data found for the given project. The initial approach was to count all extensions found inside a project. After the merge of #168 the weight is different for static file cases.
  • In order to get more info for specific modules/components that exist inside the project, one has to run component command. In the current main the component command is able to locate the root java component.
  • Looking at the response format we do include information for framework & tools. This means we're already analyzing the first dir level of the given project, so we can have different weight calculation for each case, while we don't add a performance overhead to the analyze command.
  • Another good thing to investigate in the spike issue would be to make the analyze command configurable. So the user, will be able to configure how much the static file will affect the analysis when using the cli. E.g:
alizer analyze --static-weight 15 ~/my-component

This should add 15 points of weight to all static file extensions found. As a result, in case someone doesn't want to have static files at all, they would be able to exclude them by setting the value to 0.

@shawn-hurley
Copy link
Contributor Author

Adding the test case now! thanks for the feedback!

@shawn-hurley shawn-hurley force-pushed the bug/handle-META-INF-resources branch from d70249d to 83d212c Compare August 13, 2024 12:47
@shawn-hurley
Copy link
Contributor Author

@thepetk Test case for the application that I was using to see the bug was added!

@thepetk
Copy link
Contributor

thepetk commented Aug 13, 2024

@thepetk Test case for the application that I was using to see the bug was added!

@shawn-hurley would it be possible to reduce the size of the test resource we're adding? We could keep the same ratio between java/js files and ofc all config files. You could check the rest of the test resources to use as an example.

@shawn-hurley
Copy link
Contributor Author

I think the problem was the size of the js files, does the app not have to compile or work in anyway?

@thepetk
Copy link
Contributor

thepetk commented Aug 13, 2024

I think the problem was the size of the js files, does the app not have to compile or work in anyway?

No not at all. It won't be compiled. We need only a "structure" of a project that alizer can work on. So for this case we can keep all config files, so alizer can detect the framework etc and a smaller amount of js / java files to analyze the language.

@shawn-hurley shawn-hurley force-pushed the bug/handle-META-INF-resources branch from 83d212c to edb0349 Compare August 13, 2024 13:22
@shawn-hurley
Copy link
Contributor Author

is this better?

@thepetk
Copy link
Contributor

thepetk commented Aug 13, 2024

is this better?

@shawn-hurley I think much better, I'd also remove any Dockerfile, Images, License, Documentation and Kubernetes related items so the test project contains only java & js files. This way we keep alizer repo minimal while we're focusing on the specific case we've met on the test resource!

@shawn-hurley shawn-hurley force-pushed the bug/handle-META-INF-resources branch from edb0349 to 09732af Compare August 13, 2024 13:34
@shawn-hurley
Copy link
Contributor Author

I think I got them all, sorry for all the extra info and stuff

@shawn-hurley shawn-hurley force-pushed the bug/handle-META-INF-resources branch from 09732af to badfcbb Compare August 13, 2024 13:36
@thepetk
Copy link
Contributor

thepetk commented Aug 13, 2024

@shawn-hurley I've tried to remove more items (Docs, Images, Html, Css, License files). Added a commit in your branch. Feel free to revert if you think is not needed.

@shawn-hurley
Copy link
Contributor Author

No thank you, I didn't think about looking in the components directory itself sorry!

Copy link
Contributor

@thepetk thepetk 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 Nice fix!

@michael-valdron I've created devfile/api#1622 for the investigation we were discussing.

Copy link

openshift-ci bot commented Aug 13, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shawn-hurley, thepetk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@thepetk thepetk merged commit 464fff4 into devfile:main Aug 13, 2024
6 checks passed
@thepetk
Copy link
Contributor

thepetk commented Aug 13, 2024

@shawn-hurley I've also cut alizer v1.6.1 release which includes the fix we've just merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants