Skip to content

Conversation

wujimin
Copy link
Contributor

@wujimin wujimin commented Jun 16, 2017

No description provided.

wujimin added 6 commits June 17, 2017 00:20
2.spring boot springmvc can not run
1.can read multiple microservice.yaml from classpath
2.collection microservice definition data
3.do not make change to dynamic data.
convert config to micoservice definition
1. avoid start spring context
2. delete file after UT.
1. avoid start spring context
2. delete file after UT.
…/java-chassis into multi-microservice-configuration
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 81.668% when pulling 0bc4222 on wujimin:multi-microservice-configuration into ce5c10f on ServiceComb:master.


sort();
} catch (IOException e) {
throw new RuntimeException("Failed to load microservice config.", e);
Copy link
Member

Choose a reason for hiding this comment

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

we shall use custom exception instead of runtime exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only happened when startup
just stop the start process, it's no problem.

if (configModelList.isEmpty()) {
LOGGER.warn("No URLs will be polled as dynamic configuration sources.");
LOGGER.info("To enable URLs as dynamic configuration sources, define System property "
+ ADDITIONAL_CONFIG_URL + " or make " + configFileFromClasspath
Copy link
Member

Choose a reason for hiding this comment

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

string concat in logger message is not preferred, please use LOGGER.info("some message from {}", someOne);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

only happened when startup.
someone copy from com.netflix.config.sources.URLConfigurationSource.URLConfigurationSource() months ago.
change to {} or not, both no problem.

}

// only for unittest
public class YAMLConfigurationSource extends MicroserviceConfigurationSource {
Copy link
Member

Choose a reason for hiding this comment

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

if only for unit test, why not move to src/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.

io.servicecomb.foundation.ssl.SSLOptionTest
used this class too.
i don't want to copy this to another project.

Copy link
Member

Choose a reason for hiding this comment

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

we can build test-jar to other modules...

for (ConfigModel configModel : loader.getConfigModelList()) {
sb.append(configModel.getUrl()).append(",");
}
Assert.assertEquals("jar:j2,jar:j3,jar:j4,jar:j1,file:f2,file:f3,file:f4,file:f1,", sb.toString());
Copy link
Member

Choose a reason for hiding this comment

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

collect models into a list and assertThat(list, contains("jar:j2", "jar:j3") is more readable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is for sort result, it's must equal, not contains

Copy link
Member

Choose a reason for hiding this comment

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

contains checks for exact match of elements...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

contains is indexOf

public boolean matches(Object actual) {  
    return actual != null && ((String) actual).contains(substring);  
}  


public boolean contains(CharSequence s) {  
    return indexOf(s.toString()) > -1;  
}  

Copy link
Member

Choose a reason for hiding this comment

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

not this one, what i talked about is contains or containsInAnyOrder from hamcrest lib

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe.
but i feel that, my code is clear more.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 81.826% when pulling 117fee5 on wujimin:multi-microservice-configuration into 2e3caa5 on ServiceComb:master.

@WillemJiang
Copy link
Member

The PR was merged into master, so we can close it.

@wujimin wujimin deleted the multi-microservice-configuration branch June 28, 2017 03:04
@wujimin wujimin changed the title Multi microservice configuration JAV-99 Multi microservice configuration Oct 12, 2017
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.

4 participants