Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Update ApolloApplicationContextInitializer.java #37

Closed
wants to merge 5 commits into from

Conversation

Dr-wgy
Copy link

@Dr-wgy Dr-wgy commented Aug 25, 2023

What's the purpose of this PR

#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
Copy link

github-actions bot commented Aug 25, 2023

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.
@Dr-wgy
@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
Copy link
Member

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

@Dr-wgy
Copy link
Author

Dr-wgy commented Aug 30, 2023

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

@Dr-wgy
Copy link
Author

Dr-wgy commented Aug 30, 2023

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
Copy link

codecov bot commented Sep 1, 2023

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%) ⬆️
...framework/apollo/openapi/dto/OpenCreateAppDTO.java 37.50% <37.50%> (ø)
...ring/boot/ApolloApplicationContextInitializer.java 91.66% <100.00%> (+0.14%) ⬆️
...openapi/client/service/AbstractOpenApiService.java 97.05% <100.00%> (+3.30%) ⬆️
...ollo/openapi/client/service/AppOpenApiService.java 63.63% <100.00%> (+16.96%) ⬆️

... and 1 file with indirect coverage changes

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

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.

@Dr-wgy
Copy link
Author

Dr-wgy commented Sep 1, 2023

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

@Dr-wgy Dr-wgy deleted the branch apolloconfig:main September 1, 2023 05:12
@Dr-wgy Dr-wgy closed this Sep 1, 2023
@Dr-wgy Dr-wgy deleted the main branch September 1, 2023 05:12
@github-actions github-actions bot locked and limited conversation to collaborators Sep 1, 2023
@Dr-wgy Dr-wgy restored the main branch September 1, 2023 05:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants