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 resolveEnvironment method to get proper envName from fileName #532

Conversation

atrzcinski
Copy link
Contributor

With the existing code, the value of the envName variable will never be resolved correctly because the resolveEnvironment method will get the value of the fileName variable.

For example: if the name of the configuration file is my-service-prod.yaml (where prod is the name of the environment) and we provide fileName as a parameter (value is my-service-prod) instead of finalName (value is my-service[alpha]), then resolveEnvironment method will not be able to find environment.

The effect is that consul will overwrite the environment configuration with the standard configuration (e.g. my-service.yaml), i.e. the environment configuration will not be considered appropriate.

@CLAassistant
Copy link

CLAassistant commented Oct 19, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@jeremyg484 jeremyg484 left a comment

Choose a reason for hiding this comment

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

This needs corresponding updates to the tests.

It looks like https://github.com/micronaut-projects/micronaut-discovery-client/blob/master/discovery-client/src/test/groovy/io/micronaut/discovery/consul/ConsulMockConfigurationClientFilesSpec.groovy would be the place to start.

I'd like to see the test updated so that it fails without the change to ConsulConfigurationClient, but passes with it.

@atrzcinski
Copy link
Contributor Author

atrzcinski commented Nov 16, 2023

Sorry for delay with writing tests. I implemented second test with v2 suffix. It shows, that different application-specific file name affects the final effect, so order of propertySources are different. With this fix, the priority parameter of the propertySource will be valid, so sorting will always be deterministic.

@atrzcinski
Copy link
Contributor Author

Hi,

@jeremyg484 When do you plan to merge this change?

@jeremyg484 jeremyg484 merged commit 04cbe58 into micronaut-projects:master Jan 12, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants