-
Notifications
You must be signed in to change notification settings - Fork 14
separate spring boot and spring cloud from spring detection #229
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
Conversation
Signed-off-by: Hao Zhang <haozhan@microsoft.com>
|
@zhoufenqin: changing LGTM is restricted to collaborators In response to this: 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-sigs/prow repository. |
thepetk
left a comment
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.
Hey @haoozhang thank you for your contribution!
That's a nice enhancement you're suggesting. Just to mention my approach to this would
be to just expand the logic of the springDetector, as I see the three implementations are quite similar.
So I would add different cases here (one of spring, spring boot or spring cloud)
My other comment is about test cases. It would be awesome if you could add some test cases (similar to what we have right now for spring) so we are sure that alizer after the updates detects correctly the cloud and boot frameworks all the time.
Signed-off-by: Hao Zhang <haozhan@microsoft.com>
Signed-off-by: Hao Zhang <haozhan@microsoft.com>
Signed-off-by: Hao Zhang <haozhan@microsoft.com>
Thanks for your comment, I updated the PR, please help to review again, thanks! |
Thanks @haoozhang for the updates! One last thing I'd ask is to add test cases for the spring cloud detection too. We could simply add small resource project in the projects dir and then add a test case here: https://github.com/devfile/alizer/blob/main/test/apis/component_recognizer_test.go |
@thepetk Comment updated, please help to review again, thanks! |
Signed-off-by: Hao Zhang <haozhan@microsoft.com>
2f0bfb1 to
f1d42cc
Compare
thepetk
left a comment
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.
/ok-to-test
thepetk
left a comment
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.
/lgtm
@haoozhang huge thanks for your contribution <3
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoozhang, thepetk, zhoufenqin 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 |
Description of Changes
For a pure
Spring(non-Spring Boot) project, currently it detects asSpring, Spring Bootbecause of hardcoded frameworks.This PR is to separate the
Spring Boot,Spring CloudfromSpringdetection.Related Issue(s)
Link the GitHub/GitLab/JIRA issues that are related to this PR.
Acceptance Criteria
Testing and documentation do not need to be complete in order for this PR to be approved. However, tracking issues must be opened for missing testing/documentation.
Unit/Functional tests
Documentation
Tests Performed
Springonly, noSpring Boot.Spring,Spring Boot.How To Test
Instructions for the reviewer on how to test your changes.
Notes To Reviewer
Any notes you would like to include for the reviewer.