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

Fix 1310 for ConfigMaps #1332

Merged
merged 63 commits into from
May 2, 2023
Merged

Fix 1310 for ConfigMaps #1332

merged 63 commits into from
May 2, 2023

Conversation

wind57
Copy link
Contributor

@wind57 wind57 commented Apr 30, 2023

No description provided.

wind57 and others added 30 commits December 4, 2021 07:59
@@ -139,7 +140,8 @@ private void addPropertySourceIfNeeded(Function<String, Map<String, Object>> con
LOG.warn("Property source: " + name + "will be ignored because no properties could be found");
}
else {
composite.addFirstPropertySource(new MapPropertySource(name, map));
LOG.debug("will add file-based property source : " + name);
composite.addFirstPropertySource(new MountConfigMapPropertySource(name, map));
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 1/2 the fix, provide a well known class so that we can later check for it.

/**
* @author wind57
*/
public final class MountConfigMapPropertySource extends MapPropertySource {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of a generic MapPropertySource, use a class that can be later checked easily via instanceOf

@@ -84,14 +85,24 @@ public static <S extends PropertySource<?>> List<S> findPropertySources(Class<S>
else if (sourceClass.isInstance(source)) {
managedSources.add(sourceClass.cast(source));
}
else if (source instanceof MountConfigMapPropertySource mountConfigMapPropertySource) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the other 1/2 of the fix. Also a bit down in the code also, for the bootstrap case


/**
* <pre>
* - we have "spring.config.import: kubernetes", which means we will 'locate' property sources
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have 4 of these integration tests: with config-data and bootstrap for fabric8 and k8s-client.

@wind57 wind57 changed the title Fix 1310 Fix 1310 for ConfigMaps May 1, 2023
@wind57 wind57 marked this pull request as ready for review May 1, 2023 08:00
@wind57
Copy link
Contributor Author

wind57 commented May 1, 2023

@ryanjbaxter I have split the fix into two parts: one for ConfigMaps and one for Secrets (will work on it after these PRs). I also assume this needs to be fixed in 2.1.x and main, as such two PRs. Thank you for looking into it.

@ryanjbaxter ryanjbaxter linked an issue May 1, 2023 that may be closed by this pull request
@ryanjbaxter ryanjbaxter merged commit 944c7d7 into spring-cloud:main May 2, 2023
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.

Unable to listen for file changes
3 participants