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

feat: use can change spring.profiles.active's value without rebuild project #4616

Conversation

Anilople
Copy link
Contributor

@Anilople Anilople commented Oct 27, 2022

What's the purpose of this PR

When user want to use another registry, like consul, nacos, zookeeper etc. they need to change ${apollo_profile} in build.bat or build.sh to rebuild project, then get a new .zip file.

This PR will let user can change spring.profiles.active=xxx in configservice and adminservice's config/application.properties file without rebuild project.

Which issue(s) this PR fixes:

Fixes #4610

Brief changelog

Add a new file config/application.properties to configservice and adminservice's .zip file.

Some error will happen after do that.

Because when multiple application.properties in spring boot, only one of them will be keep, module apollo-common's application.properties is override by configservice and adminservice's.

After some research, I move module apollo-common's application.properties in to config/application.properties, it works.
(According org.springframework.boot.env.EnvironmentPostProcessorApplicationListener, DEFAULT_SEARCH_LOCATIONS in org.springframework.boot.context.config.ConfigDataEnvironmentPostProcessor)

Database H2 test will fail too. My way is add @TestPropertySource("classpath:" + "integration.test.properties") to @SpringBootTest.

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

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@Anilople
Copy link
Contributor Author

What's the function of config/app.properties, can we delete it or hide it in .jar file?

@codecov
Copy link

codecov bot commented Oct 27, 2022

Codecov Report

Merging #4616 (31c2a30) into master (4604e30) will decrease coverage by 0.04%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #4616      +/-   ##
============================================
- Coverage     47.20%   47.16%   -0.05%     
+ Complexity     1651     1649       -2     
============================================
  Files           347      347              
  Lines         10644    10644              
  Branches       1057     1057              
============================================
- Hits           5025     5020       -5     
- Misses         5315     5320       +5     
  Partials        304      304              
Impacted Files Coverage Δ
...ollo/adminservice/AdminServiceHealthIndicator.java 37.50% <0.00%> (-62.50%) ⬇️
...ervice/service/ReleaseMessageServiceWithCache.java 85.88% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nobodyiam
Copy link
Member

What's the function of config/app.properties, can we delete it or hide it in .jar file?

I suppose it's not used, we could remove it and test whether the /apollo/app works.

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.

There is one thing that I don't understand though. Now that we have one config/application.properties under the root folder and also one config/application.properties in the apollo-common.jar classpath, how come they could be effective together?

@Anilople
Copy link
Contributor Author

There is one thing that I don't understand though. Now that we have one config/application.properties under the root folder and also one config/application.properties in the apollo-common.jar classpath, how come they could be effective together?

In org.springframework.boot.context.config.ConfigDataEnvironment,

https://github.com/spring-projects/spring-boot/blob/29f7a596feae0051ced49da7b1622e390f7a42e7/spring-boot-project/spring-boot/src/main/java/org/springframework/boot/context/config/ConfigDataEnvironment.java#L88-L94

	static final ConfigDataLocation[] DEFAULT_SEARCH_LOCATIONS;
	static {
		List<ConfigDataLocation> locations = new ArrayList<>();
		locations.add(ConfigDataLocation.of("optional:classpath:/;optional:classpath:/config/"));
		locations.add(ConfigDataLocation.of("optional:file:./;optional:file:./config/;optional:file:./config/*/"));
		DEFAULT_SEARCH_LOCATIONS = locations.toArray(new ConfigDataLocation[0]);
	}

config/application.properties in the apollo-common.jar is optional:classpath:/config/,
but config/application.properties in zip file is optional:file:./config/,
so they are not cover each other?

@Anilople Anilople requested a review from nobodyiam October 28, 2022 15:23
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.

I added some suggestions to the documentation, please take a look and accept the changes if appropriate. Then we could change the English version too.

docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
docs/zh/deployment/distributed-deployment-guide.md Outdated Show resolved Hide resolved
@Anilople Anilople requested a review from nobodyiam November 6, 2022 03:25
Anilople and others added 21 commits November 6, 2022 13:29
@nobodyiam nobodyiam force-pushed the feature/spring-profile-change-without-repackage branch from 6dc7563 to 31c2a30 Compare November 6, 2022 05:29
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.

LGTM

@nobodyiam nobodyiam merged commit 821c855 into apolloconfig:master Nov 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2022
@nobodyiam nobodyiam added this to the 2.1.0 milestone Feb 5, 2023
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