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

reload properties of remotely activated profile fixes gh-1834 #1835

Closed
wants to merge 2 commits into from

Conversation

Haybu
Copy link
Contributor

@Haybu Haybu commented Mar 15, 2021

Note: This PR works only for remote client applications that use SB 2.5.x

@codecov
Copy link

codecov bot commented Mar 16, 2021

Codecov Report

Merging #1835 (8c9cf4f) into master (d06ab15) will decrease coverage by 0.12%.
The diff coverage is 30.76%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1835      +/-   ##
============================================
- Coverage     78.42%   78.29%   -0.13%     
- Complexity     1304     1306       +2     
============================================
  Files           165      165              
  Lines          4778     4791      +13     
  Branches        641      645       +4     
============================================
+ Hits           3747     3751       +4     
- Misses          774      781       +7     
- Partials        257      259       +2     
Impacted Files Coverage Δ Complexity Δ
...fig/client/ConfigServicePropertySourceLocator.java 63.38% <30.76%> (-3.29%) 22.00 <2.00> (+2.00) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d06ab15...8c9cf4f. Read the comment docs.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

Maybe an test? or you could show me a failing sample?

public ConfigServicePropertySourceLocator(ConfigClientProperties defaultProperties) {
this.defaultProperties = defaultProperties;
}

@Override
@Retryable(interceptor = "configServerRetryInterceptor")
Copy link
Member

Choose a reason for hiding this comment

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

I think it needs to stay here for some rare backwards compatibility issues since the call to locate(properties) wouldn't be retryable.

@ryanjbaxter
Copy link
Contributor

Should we wait until we fork the repo since this is only compatible with boot 2.5.x? How would we test it?

@spencergibb
Copy link
Member

yeah, this will have to wait a bit. Not sure we're going to fork for 2.5.x. There should be compatibility tests. Maybe there's a way to only run a test only for a specific version of boot.

}

@Retryable(interceptor = "configServerRetryInterceptor")
public org.springframework.core.env.PropertySource<?> locate(ConfigClientProperties properties) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the only usage of this the method above?

// to check if different profile is activated within the default profile
private boolean checkIfProfileIsActivatedInDefault(String[] profiles, Map<String, Object> map) {
List<String> profilesList = Arrays.asList(profiles);
return (profilesList.size() == 1 && profilesList.get(0).equalsIgnoreCase("default")
Copy link
Contributor

Choose a reason for hiding this comment

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

@spencergibb do you recall why this would only need to be done for the default profile? What if the default profile contains spring.profiles.active and then subsequent configuration properties which get activated based on that profile also contain spring.profiles.activate? In that case we would not also activate those profiles.

@ryanjbaxter
Copy link
Contributor

Fixed by #2260

@ryanjbaxter ryanjbaxter closed this May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Activate a different profile from within a default one
4 participants