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

Fixed the order of importing properties #882

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

tomek82
Copy link
Contributor

@tomek82 tomek82 commented Sep 12, 2023

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Change the Secrets Manager resolver to use resolve instead of resolveProfileSpecific to fix the import order bug.

💡 Motivation and Context

#881

💚 How did you test it?

Build locally and run application with
spring.config.import=aws-secretsmanager:${ENV}/config,optional:classpath:config.properties
and verified that the properties are overridden correctly. Also tested with profile-specific application-local.properties.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions github-actions bot added the component: secrets-manager Secrets Manager integration related issue label Sep 12, 2023
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Thanks @tomek82. Good catch. Can you add a test for this change?

@tomek82
Copy link
Contributor Author

tomek82 commented Oct 23, 2023

@maciejwalkowiak What is a good way to test it? My attempts with adding a classpath source to command line in io.awspring.cloud.autoconfigure.config.secretsmanager.SecretsManagerConfigDataLoaderIntegrationTests fail as it seems to be working without this change and I can't get a separate @propertysource to work either (it does load the file but doesn't process the secrets import).

@maciejwalkowiak
Copy link
Contributor

You can test it like this.

Add test to SecretsManagerConfigDataLoaderIntegrationTests:

@Test
void respectsImportOrder() {
	SpringApplication application = new SpringApplication(App.class);
	application.setWebApplicationType(WebApplicationType.NONE);

	try (ConfigurableApplicationContext context = runApplication(application,
		"aws-secretsmanager:/config/spring,classpath:config.properties")) {
		assertThat(context.getEnvironment().getProperty("another-parameter")).isEqualTo("from properties file");
	}
}

and config.properties file in src/test/resources:

another-parameter=from properties file

The test passes without your changes too, so I am a bit puzzled. Perhaps this bug exists only in 2.x?

@tomek82
Copy link
Contributor Author

tomek82 commented Oct 30, 2023

I've added a test with one more level of indirection, without my fix this fails to load the properties in the right order.

@maciejwalkowiak maciejwalkowiak modified the milestones: 3.0.3, 3.1.0 Nov 3, 2023
Copy link
Contributor

@maciejwalkowiak maciejwalkowiak left a comment

Choose a reason for hiding this comment

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

Thanks! Because it may have surprising side effects we will release it with 3.1.0

@maciejwalkowiak maciejwalkowiak merged commit bb94631 into awspring:main Dec 1, 2023
4 checks passed
@maciejwalkowiak maciejwalkowiak added the type: bug Something isn't working label Dec 1, 2023
maciejwalkowiak pushed a commit that referenced this pull request Dec 10, 2023
…882)

Co-authored-by: Tomek Bielecki <Tomek.Bielecki@aus.com>
maciejwalkowiak pushed a commit that referenced this pull request Dec 10, 2023
…882)

Co-authored-by: Tomek Bielecki <Tomek.Bielecki@aus.com>
maciejwalkowiak added a commit that referenced this pull request Dec 10, 2023
maciejwalkowiak added a commit that referenced this pull request Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: secrets-manager Secrets Manager integration related issue type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants