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

[RFR] Fix Apache wicket app analysis failure #213

Merged
merged 8 commits into from
Nov 20, 2024

Conversation

nachandr
Copy link
Collaborator

No description provided.

@nachandr nachandr requested a review from aufi November 15, 2024 23:19
@nachandr nachandr changed the title [WIP] Fix Apache wicket app analysis failure [RFR] Fix Apache wicket app analysis failure Nov 15, 2024
@nachandr
Copy link
Collaborator Author

Apache_wicket

Copy link
Collaborator

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

Thanks @nachandr for the PR.
Could you please add analysis tag and dependencies checks?

Copy link
Collaborator

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

@nachandr please create a new application var for this analysis in https://github.com/konveyor/go-konveyor-tests/blob/main/data/application.go

analysis/tc_apache_wicket.go Outdated Show resolved Hide resolved
@nachandr
Copy link
Collaborator Author

Thanks @nachandr for the PR. Could you please add analysis tag and dependencies checks?

Analysis tags and dependencies are not generated with the selected target.

@mguetta1
Copy link
Collaborator

mguetta1 commented Nov 18, 2024

Thanks @nachandr for the PR. Could you please add analysis tag and dependencies checks?

Analysis tags and dependencies are not generated with the selected target.

Good, please set the expected result to an empty array so if something changes we can catch it - Dependencies: []api.TechDependency{}
Thanks!

Thanks @mguetta1. Just want to make sure I have this right. Are you referring to a placeholder for the future or something else? If it's just a placeholder , I think we could leave the code as is and add the dependencies when they do get generated.

Signed-off-by: Nandini Chandra <[email protected]>
@mguetta1
Copy link
Collaborator

Thanks @nachandr for the PR. Could you please add analysis tag and dependencies checks?

Analysis tags and dependencies are not generated with the selected target.

Good, please set the expected result to an empty array so if something changes we can catch it - Dependencies: []api.TechDependency{} Thanks!

Thanks @mguetta1. Just want to make sure I have this right. Are you referring to a placeholder for the future or something else? If it's just a placeholder , I think we could leave the code as is and add the dependencies when they do get generated.

Hi, not a placeholder but an empty array to make sure that this is the expected behavior instead of skipping the Dependencies check

@mguetta1 mguetta1 added cherry-pick/release-0.5 This PR should be cherry-picked to release-0.5 branch. cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch labels Nov 19, 2024
@nachandr
Copy link
Collaborator Author

Thanks @nachandr for the PR. Could you please add analysis tag and dependencies checks?

Analysis tags and dependencies are not generated with the selected target.

Good, please set the expected result to an empty array so if something changes we can catch it - Dependencies: []api.TechDependency{} Thanks!
Thanks @mguetta1. Just want to make sure I have this right. Are you referring to a placeholder for the future or something else? If it's just a placeholder , I think we could leave the code as is and add the dependencies when they do get generated.

Hi, not a placeholder but an empty array to make sure that this is the expected behavior instead of skipping the Dependencies check

Sure, I could do that in a follow up PR . I'd also like to check if analysis_test.go needs to be updated.

@nachandr
Copy link
Collaborator Author

Thanks @nachandr for the PR. Could you please add analysis tag and dependencies checks?

Analysis tags and dependencies are not generated with the selected target.

Good, please set the expected result to an empty array so if something changes we can catch it - Dependencies: []api.TechDependency{} Thanks!
Thanks @mguetta1. Just want to make sure I have this right. Are you referring to a placeholder for the future or something else? If it's just a placeholder , I think we could leave the code as is and add the dependencies when they do get generated.

Hi, not a placeholder but an empty array to make sure that this is the expected behavior instead of skipping the Dependencies check

Sure, I could do that in a follow up PR . I'd also like to check if analysis_test.go needs to be updated.

#221

Copy link
Collaborator

@mguetta1 mguetta1 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

mguetta1
mguetta1 previously approved these changes Nov 19, 2024
@mguetta1 mguetta1 self-requested a review November 19, 2024 18:44
@mguetta1 mguetta1 dismissed their stale review November 19, 2024 18:46

The change looks good to me. I am not sure that the current analysis result is really the expected result (no tags/deps)

@mguetta1 mguetta1 removed their request for review November 19, 2024 18:46
@nachandr
Copy link
Collaborator Author

nachandr commented Nov 19, 2024

Hi @aufi and @pranavgaikwad ,
Dependencies and Tags were not found during the analysis of the Apache Wicket app with target=cloud-readiness .
At some point, a few Analysis tags would get generated, but they don't generated anymore (as seen in this PR).

Is this expected?

Copy link
Member

@aufi aufi left a comment

Choose a reason for hiding this comment

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

Hi @nachandr, this test case specifies path within the source repository, so looking on the sample app: https://github.com/windup/windup-sample-apps/tree/master/test-files/src_example/org/apache/wicket, no dependencies is (probably) correct, there is no pom.xml in the directory subtree.

Generally, there should be tags at least from language discovery (Java and Maven for this app), but these come from language-discovery task triggered automatically with application creation.
So, they might have appeared in case analysis has finished later than the language discovery. This is not common on most applications, but here, the application looks to be just a bunch of .java files, so analysis might be pretty fast. This could explain sometimes generated tags.

I tested the analysis locally, except the discovery tags, results are the same and since this is analysis tests more that discovery tests, this PR with empty Dependencies and Tags is LGTM.

@nachandr
Copy link
Collaborator Author

Hi @nachandr, this test case specifies path within the source repository, so looking on the sample app: https://github.com/windup/windup-sample-apps/tree/master/test-files/src_example/org/apache/wicket, no dependencies is (probably) correct, there is no pom.xml in the directory subtree.

Generally, there should be tags at least from language discovery (Java and Maven for this app), but these come from language-discovery task triggered automatically with application creation. So, they might have appeared in case analysis has finished later than the language discovery. This is not common on most applications, but here, the application looks to be just a bunch of .java files, so analysis might be pretty fast. This could explain sometimes generated tags.

I tested the analysis locally, except the discovery tags, results are the same and since this is analysis tests more that discovery tests, this PR with empty Dependencies and Tags is LGTM.

@aufi , Thank you !

@nachandr nachandr merged commit e5579aa into konveyor:main Nov 20, 2024
6 of 7 checks passed
github-actions bot pushed a commit that referenced this pull request Nov 20, 2024
* Update Analysis issues

Signed-off-by: Nandini Chandra <[email protected]>

* Update Incidents

Signed-off-by: Nandini Chandra <[email protected]>

* Update Incidents for Analysis issues

Signed-off-by: Nandini Chandra <[email protected]>

* Move application initilaization out of test file

Signed-off-by: Nandini Chandra <[email protected]>

* Move application initilaization out of test file

Signed-off-by: Nandini Chandra <[email protected]>

* Remove redundant lines

Signed-off-by: Nandini Chandra <[email protected]>

* Import required packages

Signed-off-by: Nandini Chandra <[email protected]>

---------

Signed-off-by: Nandini Chandra <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
mguetta1 pushed a commit that referenced this pull request Nov 21, 2024
Signed-off-by: Nandini Chandra <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Nandini Chandra <[email protected]>
github-actions bot pushed a commit that referenced this pull request Nov 21, 2024
* Update Analysis issues

Signed-off-by: Nandini Chandra <[email protected]>

* Update Incidents

Signed-off-by: Nandini Chandra <[email protected]>

* Update Incidents for Analysis issues

Signed-off-by: Nandini Chandra <[email protected]>

* Move application initilaization out of test file

Signed-off-by: Nandini Chandra <[email protected]>

* Move application initilaization out of test file

Signed-off-by: Nandini Chandra <[email protected]>

* Remove redundant lines

Signed-off-by: Nandini Chandra <[email protected]>

* Import required packages

Signed-off-by: Nandini Chandra <[email protected]>

---------

Signed-off-by: Nandini Chandra <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
mguetta1 pushed a commit that referenced this pull request Nov 21, 2024
Signed-off-by: Nandini Chandra <[email protected]>
Signed-off-by: Cherry Picker <[email protected]>
Co-authored-by: Nandini Chandra <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick/release-0.5 This PR should be cherry-picked to release-0.5 branch. cherry-pick/release-0.6 This PR should be cherry-picked to release-0.6 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants