apolloconfig / apollo-java

Apollo Java Clients
Apache License 2.0
40 stars 72 forks source link

Update ApolloApplicationContextInitializer.java #37

Closed Dr-wgy closed 1 year ago

Dr-wgy commented 1 year ago

What's the purpose of this PR

https://github.com/apolloconfig/apollo-java/issues/36

Which issue(s) this PR fixes:

Fixes #

I want fix

when we use spring-cloud-config to config app.id and apollo.meta apollo.cluster

the app.id is ApolloNoAppIdPlaceHolder

Brief changelog

support spring-cloud-config to config APOLLO_SYSTEM_PROPERTIES

public static final String[] APOLLO_SYSTEM_PROPERTIES = {ApolloClientSystemConsts.APP_ID, ApolloClientSystemConsts.APOLLO_LABEL, ApolloClientSystemConsts.APOLLO_CLUSTER, ApolloClientSystemConsts.APOLLO_CACHE_DIR, ApolloClientSystemConsts.APOLLO_ACCESS_KEY_SECRET, ApolloClientSystemConsts.APOLLO_META, ApolloClientSystemConsts.APOLLO_CONFIG_SERVICE, ApolloClientSystemConsts.APOLLO_PROPERTY_ORDER_ENABLE, ApolloClientSystemConsts.APOLLO_PROPERTY_NAMES_CACHE_ENABLE, ApolloClientSystemConsts.APOLLO_OVERRIDE_SYSTEM_PROPERTIES};

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


1 out of 2 committers have signed the CLA.
:white_check_mark: @Dr-wgy
:x: @wuguanyu
wuguanyu seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You can retrigger this bot by commenting recheck in this Pull Request

nobodyiam commented 1 year ago

This change was introduced in https://github.com/apolloconfig/apollo/pull/1614, would you please help to first check that pr and verify what the impact is?

Dr-wgy commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

Dr-wgy commented 1 year ago

This change was introduced in apolloconfig/apollo#1614, would you please help to first check that pr and verify what the impact is? image image

I check logic if I want get APOLLO_SYSTEM_PROPERTIES meta-data from spring-cloud-config. So I need retry to execute
initializeSystemProperty(environment) I think it doesn't afffect logSystem load config from apollo

if apollo system properties already been initialized, it doesn't affect system property when we re-execute initializeSystemProperty

` private void fillSystemPropertyFromEnvironment(ConfigurableEnvironment environment, String propertyName) { if (System.getProperty(propertyName) != null) { return; }

String propertyValue = environment.getProperty(propertyName);

if (Strings.isNullOrEmpty(propertyValue)) {
  return;
}

System.setProperty(propertyName, propertyValue);

}`

codecov[bot] commented 1 year ago

Codecov Report

Merging #37 (710aa76) into main (d2eca1f) will increase coverage by 0.23%. Report is 1 commits behind head on main. The diff coverage is 63.88%.

@@             Coverage Diff              @@
##               main      #37      +/-   ##
============================================
+ Coverage     68.35%   68.59%   +0.23%     
- Complexity     1193     1206      +13     
============================================
  Files           169      171       +2     
  Lines          5161     5197      +36     
  Branches        561      561              
============================================
+ Hits           3528     3565      +37     
- Misses         1369     1371       +2     
+ Partials        264      261       -3     
Files Changed Coverage Δ
...ramework/apollo/openapi/api/AppOpenApiService.java 0.00% <0.00%> (ø)
...ork/apollo/openapi/client/ApolloOpenApiClient.java 54.83% <0.00%> (+8.17%) :arrow_up:
...framework/apollo/openapi/dto/OpenCreateAppDTO.java 37.50% <37.50%> (ø)
...ring/boot/ApolloApplicationContextInitializer.java 91.66% <100.00%> (+0.14%) :arrow_up:
...openapi/client/service/AbstractOpenApiService.java 97.05% <100.00%> (+3.30%) :arrow_up:
...ollo/openapi/client/service/AppOpenApiService.java 63.63% <100.00%> (+16.96%) :arrow_up:

... and 1 file with indirect coverage changes

Dr-wgy commented 1 year ago

Can we add some unit tests to verify this logic and prevent it from being mistakenly modified in the future?

BTW, please also update the CHANGES.md.

ok