-
Notifications
You must be signed in to change notification settings - Fork 665
Add an example of using Jersey in karaf #633
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
|
The failure is: I put the same copyright headers on files that I found in other example projects. If that was wrong, tell me what to use and I will use it. |
|
The following files don't contain the ASF headers: Can you please fix that ? |
|
The failure cause of the jenkins build this time, was a failing iTest that should not be affected by this new example: |
|
Yes, we have a flacky test on the example. We will fix it. |
|
Do I need to merge in the newest from master? |
f80cacb to
d2c9c21
Compare
|
I don't understand the current jenkins errors. There seems to be a repository confusion error that I don't know how to fix (jenkins tries to fetch from the karaf repository, instead of the repository where my branch lives). |
|
@steinarb yes, it's a random issue between Jenkins and github. No need to rebase, let me deal with that. |
jbonofre
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.
Overall good. I have some cosmetic changes I will do.
| --> | ||
|
|
||
| <modelVersion>4.0.0</modelVersion> | ||
| <artifactId>karaf-jersey-example.servicedef</artifactId> |
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.
In order to be consistent with the other example, I think it would be better to use karaf-jersey-example-service.
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.
Looking at the other examples in karaf/examples/ the service definitions seem to reside in a module with name ending in "-api", like e.g. karaf-jdbc-example and karaf-rest-example?
The module name for the bundle providing the service implementation for the two examples mentioned above, ends with "-provider". Is this what I should use as well?
What about the webapi and webgui modules? Should I keep the names, but replace "." with "-"? Ie. karaf-jersey-example-webapi and karaf-jersey-example-webgui?
| <description>This is an OSGi bundle defining beans and interfaces for OSGI services.</description> | ||
|
|
||
| <properties> | ||
| <Bundle-SymbolicName>org.apache.karaf.examples.jersey.servicedef</Bundle-SymbolicName> |
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.
This property doesn't seem to be use in the maven-bundle-plugin configuration.
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.
It was supposed to be, unless I've messed up something, and I think I verified what the actual symbolic name was.
The parent POM defines BundleSymbolicName to be, in properties:
<Bundle-SymbolicName>${project.groupId}.${project.artifactId}</Bundle-SymbolicName>
and also inside the maven-bundle-plugin config (on line 301 in the top POM in my version):
<instructions>
<Bundle-SymbolicName>${Bundle-SymbolicName}</Bundle-SymbolicName>
However, when I put "-" into the artifactId I could no longer use that approach. Also the groupId contained much of the same information as the long artifactId. So I put "overrides" in each bundle project to get a sensible symbolic name, such as eg. the example above.
I've just checked the manifest.mf of the karaf-jersey-example.servicedef-4.2.2-SNAPSHOT.jar file in my build, and it contains
Bundle-SymbolicName: org.apache.karaf.examples.jersey.servicedef
(which is the setting from the properties you quote)
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.karaf.examples.jersey.servicedef.beans; |
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.
No need to put in beans. The POJO used by the service can be at the same service definition level aka org.apache.karaf.examples.jersey.service.
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've used org.apache.karaf.examples.jersey.services for the package containing the service implementation, and org.apache.karaf.examples.jersey.servicedef for the service implementation (which also has a match in part of the artifactId, the module name and bundle-symbolic-name).
Is it OK to keep this?
Or will the service definition package need to be changed to org.apache.karaf.examples.jersey.service and the implementation package need to be changed to something else?
| import org.apache.karaf.examples.jersey.servicedef.beans.Count; | ||
| import org.osgi.service.component.annotations.Component; | ||
|
|
||
| @Component(service={Counter.class}) |
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.
That's implicit with SCR, @Component is enough.
|
retest this please |
|
I'm doing a new pass, probably upgrade |
|
Sorry for the delay in the review, I'm in vacation but I will do a new pass. |
|
If you want to, I can do the suggested changes and update the PR branch? |
|
Yeah, I will provide all feedback to be consistent with the other examples. |
|
Old PR cleanup |
No description provided.