Skip to content

Fix context order when loading properties #195

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Dec 14, 2021
Merged

Fix context order when loading properties #195

merged 2 commits into from
Dec 14, 2021

Conversation

WtfJoke
Copy link
Contributor

@WtfJoke WtfJoke commented Oct 20, 2021 •
edited
Loading

Fixes gh-169

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Reverse the context order when loading properties to preserve documented order (eg service specific context "wins" over more general application context)

💡 Motivation and Context

Fixes #169

💚 How did you test it?

By JUnit Test(s) and in a sample project WtfJoke/chaoskotlindemo/awsparamterstoreconfig

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • [N/A] I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes - well its somehow breaking as it reverse the order, but imho never actually worked like intended since the (unwanted) change

🔮 Next steps

@github-actions github-actions bot added the component: parameter-store Parameter Store integration related issue label Oct 20, 2021
@WtfJoke WtfJoke mentioned this pull request Oct 20, 2021
Closed
@WtfJoke WtfJoke force-pushed the fixContextOrderIssue169 branch 2 times, most recently from 4d10a96 to a9eb37f Compare October 22, 2021 09:30
@WtfJoke WtfJoke marked this pull request as ready for review October 22, 2021 09:35
@WtfJoke WtfJoke force-pushed the fixContextOrderIssue169 branch from a9eb37f to 419e70d Compare October 22, 2021 11:39
MatejNedic
MatejNedic requested changes Oct 22, 2021
View reviewed changes
Copy link
Member

@MatejNedic MatejNedic left a comment

Choose a reason for hiding this comment

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

Hey @WtfJoke ,
thanks so much for your contribution if you could address the following comments.

Since this is an issue with SecretManager are you willing of doing the same fix?

...tore-config/src/test/java/io/awspring/cloud/paramstore/AwsParamStorePropertySourcesTest.java Outdated

@Test
void getAutomaticContextsWithSingleProfile() {
AwsParamStorePropertySources sut = new AwsParamStorePropertySources(properties, logMock);
Copy link
Member

Choose a reason for hiding this comment

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

Please rename to propertySource instead of sut

...tore-config/src/test/java/io/awspring/cloud/paramstore/AwsParamStorePropertySourcesTest.java Outdated

private final String defaultContextName = "application";

private final String defaultApplicationName = "messaging-service";
Copy link
Member

Choose a reason for hiding this comment

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

Rename to APPLICATION_NAME if we are going to keep it as constant. Since it is not used anywhere except setUp I am thinking if there is a point to have it as a constant? We probably won't use it anywhere else in the future.

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'll inline the variable

...er-store-config/src/main/java/io/awspring/cloud/paramstore/AwsParamStorePropertySources.java Outdated
@@ -52,6 +54,8 @@ public AwsParamStorePropertySources(AwsParamStoreProperties properties, Log log)

addProfiles(contexts, defaultContext, profiles);
contexts.add(defaultContext + "/");

Collections.reverse(contexts);
Copy link
Member

@MatejNedic MatejNedic Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of doing reverse, I think it is better to just put

addProfiles(contexts, defaultContext, profiles);
contexts.add(defaultContext + "/");

Before appContext insertions.
There is no point in doing reverse if we control the order of insertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, good point

@WtfJoke
Copy link
Contributor Author

WtfJoke commented Oct 22, 2021

Since this is an issue with SecretManager are you willing of doing the same fix?

I'm actually already working on that 😄 Will do, wasnt sure if I should put it on a separate or on the same PR. But I think its better in the same (except you preferr otherwise).

Will address your comments soon

@WtfJoke WtfJoke force-pushed the fixContextOrderIssue169 branch 2 times, most recently from 91b0b57 to 45b7b7d Compare October 22, 2021 15:38
@WtfJoke WtfJoke force-pushed the fixContextOrderIssue169 branch from 45b7b7d to 7bc75b9 Compare October 22, 2021 16:13
@github-actions github-actions bot added the component: secrets-manager Secrets Manager integration related issue label Oct 22, 2021
@WtfJoke
Copy link
Contributor Author

WtfJoke commented Oct 22, 2021

Hey @MatejNedic
Thanks for your fast review! The review comments should be resolved. I also made the same fix for secrets manager config.
It would be great if you could review it again.

MatejNedic
MatejNedic requested changes Oct 22, 2021
View reviewed changes
Copy link
Member

@MatejNedic MatejNedic left a comment

Choose a reason for hiding this comment

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

Thanks on making changes!

There are 2 small changes please apply them.

@WtfJoke WtfJoke force-pushed the fixContextOrderIssue169 branch from 7bc75b9 to 669a8ea Compare October 22, 2021 19:01
@WtfJoke
Copy link
Contributor Author

WtfJoke commented Oct 22, 2021

Done :)

@WtfJoke
Copy link
Contributor Author

WtfJoke commented Oct 29, 2021

@MatejNedic I dont like to ping people, but is there anything left to do in order to get this merged? 🙃

@MatejNedic
Copy link
Member

@maciejwalkowiak @eddumelendez Could you please check PR out?
It looks good to me.

@WtfJoke WtfJoke mentioned this pull request Nov 3, 2021
Open
@piotrkorlaga
Copy link

Hello, any plans for releasing this fix, soon? :)

@maciejwalkowiak maciejwalkowiak added this to the 2.3.3 milestone Dec 14, 2021
maciejwalkowiak
maciejwalkowiak approved these changes Dec 14, 2021
View reviewed changes
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 @WtfJoke for a fix! And apologies for late merge.

@maciejwalkowiak maciejwalkowiak merged commit 9390015 into awspring:2.3.x Dec 14, 2021
@WtfJoke WtfJoke deleted the fixContextOrderIssue169 branch December 14, 2021 19:19
@WtfJoke
Copy link
Contributor Author

WtfJoke commented Dec 14, 2021

Glad it got merged eventually :) Thanks for taking care!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatejNedic MatejNedic MatejNedic requested changes

@maciejwalkowiak maciejwalkowiak maciejwalkowiak approved these changes

@eddumelendez eddumelendez Awaiting requested review from eddumelendez

Assignees
No one assigned
Labels
component: parameter-store Parameter Store integration related issue component: secrets-manager Secrets Manager integration related issue
Projects
None yet
Milestone
2.3.3
Development

Successfully merging this pull request may close these issues.

spring-cloud-aws-parameter-store-config prioritizes default-context parameters over service-specific parameters
4 participants
@WtfJoke @MatejNedic @piotrkorlaga @maciejwalkowiak