-
Notifications
You must be signed in to change notification settings - Fork 669
Add support for RESTEasy4.x #265
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
|
Does this PR continue from #169? I can see code names are very similar. |
...va/org/apache/skywalking/apm/plugin/resteasy/v4/server/SynchronousDispatcherInterceptor.java
Outdated
Show resolved
Hide resolved
…of response status code
@Leibnizhu I need a confirmation about this. |
codes are copy from resteasy 3.x, and scenario test cases are from #169. what i have done:
|
|
@carlfranz If you have time, I hope you could confirm this copy/paste doesn't concern you. @Leibnizhu Thanks for the update. Let's focus on the codes and tests, once it is passed, and Carlo doesn't object, we should be good. |
|
It seems UTs fail, could you recheck? Logs are here, https://github.com/apache/skywalking-java/runs/7107222730?check_suite_focus=true |
…herInterceptor unit test
3314e93 to
0102f5a
Compare
i have fixed it , but ci action seems to be stucked…… |
|
It just waits for my manual approval, as this is your first pull request to this repo. |
|
Your UTs are still failing, please make sure you have checked all cases locally. |
Hi @wu-sheng, codes is pretty the same, as we saw before the plugin for resteasy4 differs only by the dependency |
Yes, I knew, as you mentioned in #169 (comment). This is the key point. |
1. Add defensive code SynchronousDispatcherInterceptor to avoid NPE 2. Add mocking for HttpRequest.getHttpMethod()
...va/org/apache/skywalking/apm/plugin/resteasy/v4/server/SynchronousDispatcherInterceptor.java
Outdated
Show resolved
Hide resolved
locally test pass:
|
yes, there is no problem 😎 |
CHANGESlog.