Skip to content

Conversation

weichao666
Copy link
Contributor

@weichao666 weichao666 commented Dec 23, 2017

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [JAV-XXX] Fixes bug in ApproximateQuantiles, where you replace JAV-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean install to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

1、SDK report their framework type and version when register to SC
2、Each service with specific version has a certain framework+version
3、Present the type/version information in console in Service+Version level
4、In gov service, send the different command to CC according to the type/version when doing
management

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 87.147% when pulling 17b1c77 on weichao666:JAV566CSE769modify into dcee409 on apache:master.

try {
prop.load(in);
} catch (Exception e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please log the error and throw the exception out.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using mavenVersionFor

public void testInitializedValues() {
initFramework(); //Initialize the Values
Assert.assertEquals("JAVA-CHASSIS", framework.getName());
Assert.assertEquals("0.6.0", framework.getVersion());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change the version once we release java-chassis 0.6.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make it automatically.

@@ -0,0 +1 @@
FRAMEWORK_VERSION=0.6.0 No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about change the version to xxx-test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new modification deletes this file,replace with pom.properties

/**
* Created by on 2017/12/22.
*/
public class GetFrameworkVersionFromXml {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xml???
properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

private static void loadProperties() {
InputStream in =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to close inputStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with mavenVersionFor

* Created by on 2017/12/22.
*/
public class GetFrameworkVersionFromXml {
private static Properties prop;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need this field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


private static void loadProperties() {
InputStream in =
GetFrameworkVersionFromXml.class.getResourceAsStream("/config/frameworkVersion.properties");
Copy link
Contributor

@wujimin wujimin Dec 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found that maven will put \META-INF\maven{groupId}{artifactId}\pom.properties and pom.xml into jar automatically
we no need to create a new properties.

you can read code: com.fasterxml.jackson.core.util.VersionUtil.mavenVersionFor(ClassLoader, String, String)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@wujimin wujimin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did not find the confirm: will we use ServiceComb or cse version?
have i missed some information?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 87.086% when pulling a251d63 on weichao666:JAV566CSE769modify into dcee409 on apache:master.

@@ -0,0 +1,3 @@
groupId=io.servicecomb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we need this file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in mavenVersionFor's source code,it need to read pom.properties,
public static Version mavenVersionFor(ClassLoader classLoader, String groupId, String artifactId) {
InputStream pomPoperties = classLoader.getResourceAsStream("META-INF/maven/" +
groupId.replaceAll("\.", "/") + "/" + artifactId + "/pom.properties");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember this file will be generated automatically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes,when mvn package,this file will be generated automatically, i'll delete it

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.381% when pulling 59ad3b5 on weichao666:JAV566CSE769modify into dcee409 on apache:master.

@weichao666
Copy link
Contributor Author

After discussion, use ServiceComb version

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.451% when pulling 0f3d9cf on weichao666:JAV566CSE769modify into dcee409 on apache:master.

pom.xml Outdated
</plugin>
</plugins>
</pluginManagement>
<resources>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?
to generate pom.properties?

it seems that no need to do this.


public class TestGetFrameworkVersion {

ClassLoader classLoader = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the field purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete it now

@Test
public void test() {
@SuppressWarnings("deprecation")
Version version = VersionUtil.mavenVersionFor(classLoader, "io.servicecomb", "java-chassis");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your test target is "GetFrameworkVersion"
but where is your target instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done,GetFrameworkVersion have been deleted

/**
* Created by on 2017/12/22.
*/
public class GetFrameworkVersion {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your class name is getXXXX????

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done,delete it now,use default value first

private Framework framework;

@JsonProperty(value = "registerBy")
private String registeredBy;
Copy link
Contributor

@wujimin wujimin Jan 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the original name
i remember wrote comment, class Microservice not only describe self, but also describe others

why did not do any response?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


public class TestFramework {

Framework framework = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think 27~37 equals

Framework framework = new Framework();


public static final String GROUPID = "io.servicecomb";

public static final String ARTIFACTID = "java-chassis";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure you can read version by this artifactid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when mvn package, the version can be read,

@wujimin
Copy link
Contributor

wujimin commented Jan 4, 2018

maybe this is simpler and not depend on jackson Deprecated method:

      <plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-jar-plugin</artifactId>
        <configuration>
          <archive>
            <manifest>
              <addDefaultImplementationEntries>true</addDefaultImplementationEntries>
              <addDefaultSpecificationEntries>true</addDefaultSpecificationEntries>
            </manifest>
          </archive>
        </configuration>
      </plugin>

xxxClass.class.getPackage().getImplementationVersion()

but you need to do more test at least include:
1.web container, eg: tomcat
2.maybe openjdk

@weichao666 weichao666 changed the title [JAV-566] report type/version when register to SC [SCB-171] report type/version when register to SC Jan 5, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.47% when pulling a31b494 on weichao666:JAV566CSE769modify into dcee409 on apache:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 87.47% when pulling a31b494 on weichao666:JAV566CSE769modify into dcee409 on apache:master.


String CONFIG_FRAMEWORK_DEFAULT_NAME = "servicecomb-java-chassis";

String CONFIG_FRAMEWORK_DEFAULT_VERSION = "1.0";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fake "1.0" is not good

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set version to empty now

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 87.521% when pulling 3a4171d on weichao666:JAV566CSE769modify into e812ad7 on apache:master.

@WillemJiang WillemJiang merged commit 1fb0be2 into apache:master Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants