Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Animal Sniffer Plugin applied for Java compatibility test #1833

Merged
merged 1 commit into from
Jun 10, 2016

Conversation

jaemin-shin
Copy link
Contributor

@jaemin-shin jaemin-shin commented Jun 9, 2016

  • Now the plugin tests whether APIs used for pinpoint are compatible for java 1.6.
  • Two files are modified: pom.xml, web/pom.xml
  • This test automatically runs during project build. (ex command/ mvn install -Dmaven.test.skip=true)

**pinpoint-web is tested if it is compatible with java 1.7
*
*sun libraries are ignored at the test.

naver/kaist-oss-course#65

@Xylus
Copy link
Contributor

Xylus commented Jun 9, 2016

@999fg
Looks good!
One possible suggestion would be to pull sniffer artifactId out as a maven property.
This way, web's pom.xml could simply overwrite this property to java17 instead of redefining the whole plugin. (Similar to how the maven-compiler-plugin is using jdk.version to configure source and target levels)

Automatically setting sniffer's artifactId based on jdk.version would be even better, but I'm not sure if this can be done easily.

I'd love like to hear what you think, please let me know.
Thanks!

…ed. Now the plugin tests whether apis used for pinpoint are compatible for java 1.6.(pinpoint-web is tested if it is compatible with java 1.7.) This test automatically runs during project build.

artifactId and execution id of animal sniffer plugin turned into use the property {sniffer.artifactid} and pre-defined property {jdk.version}. This way, there is no need to re-define the plugin application at the apis which use the different version of java. What is only needed is to define {sniffer.artifactid} each case.
xx
@jaemin-shin
Copy link
Contributor Author

@Xylus

Thanks for the comment!
I totally agree with your suggestion, which is more good way of coding and compatible in further development.
When I sent this PR few hours ago, I personally did not like two lines of log appearing when pom-web is being compiled (possibly due to re-defined plugin application in two pom.xml file). Following your suggestions solved this problem.
Anyway, I updated the commit as your suggestion.
Thanks!

@Xylus Xylus merged commit 5b670e8 into pinpoint-apm:master Jun 10, 2016
@emeroad emeroad added this to the 1.6.0 milestone Jun 10, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants