[GitHub] [phoenix] stoty commented on a change in pull request #887: PHOENIX-6143: Get Phoenix Tracing Webapp work

[GitBox <gi...@apache.org>]

stoty commented on a change in pull request #887:
URL: https://github.com/apache/phoenix/pull/887#discussion_r493232427



##########
File path: phoenix-tracing-webapp/pom.xml
##########
@@ -155,6 +131,7 @@
         <plugin>
           <groupId>com.github.searls</groupId>
           <artifactId>jasmine-maven-plugin</artifactId>
+          <version>2.2</version>

Review comment:
       You are overriding the version set in the master pom pluginManagement section.
   I suggest that you override the plugin version there instead.

##########
File path: phoenix-tracing-webapp/src/build/trace-server-runnable.xml
##########
@@ -43,7 +43,19 @@
       <outputDirectory>/</outputDirectory>
       <includes>
         <include>org.apache.phoenix:phoenix-tracing-webapp</include>

Review comment:
       Ideally, this should be done by the shade plugin instead. (Not required for this patch, just a remark)

##########
File path: phoenix-tracing-webapp/README.md
##########
@@ -3,10 +3,10 @@
  *mvn clean install*

Review comment:
       Nit: We use backticks to format command lines elsewhere, might as well switch to that here, as well.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org