Skip to content

Conversation

vnandwana
Copy link
Contributor

  • Added getPropertiesBySource() contract to OSGIConfigProperties interface
  • Added implementation in DefaultKillbillConfigSource and OSGIConfigPropertiesService to expose runtime resolved properties grouped by config source.
  • Added unit test to validate grouped property retrieval.

…rface

- Added implementation in DefaultKillbillConfigSource and OSGIConfigPropertiesService to expose runtime resolved properties grouped by config source.
- Added unit test to validate grouped property retrieval.
@vnandwana vnandwana requested review from pierre and sbrossie May 27, 2025 11:16
Copy link
Member

@sbrossie sbrossie left a comment

Choose a reason for hiding this comment

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

Regarding adding a new interface OSGIConfigProperties#getPropertiesBySource, can you add in the PR comment the result of the test to verify binary compatibility: An existing plugin compiled prior this change should still work after this change. Looking at our code, we typically make this artifact provided, so I would expect to even see the plugin have access to new method with 0 recompilation. Could you confirm?

vnandwana added 2 commits June 4, 2025 18:14
* Included "AwsSsmProperties" category in getPropertiesBySource().
* Categorized properties by source: system, env, killbill, runtime, ssm, etc.
* Added helper methods to support property merging and filtering
* Updated getProperties() to ensure all sources are loaded
* Added required AWS SDK v2 dependencies for SSM fetch.
@vnandwana vnandwana requested a review from sbrossie June 5, 2025 04:41
@vnandwana
Copy link
Contributor Author

Regarding adding a new interface OSGIConfigProperties#getPropertiesBySource, can you add in the PR comment the result of the test to verify binary compatibility: An existing plugin compiled prior this change should still work after this change. Looking at our code, we typically make this artifact provided, so I would expect to even see the plugin have access to new method with 0 recompilation. Could you confirm?

For testing, I successfully installed an existing Aviate plugin using API calls without any issues. However, to access the newly added OSGIConfigProperties#getPropertiesBySource method from a plugin, we need to upgrade the killbill-oss-parent version and recompile the plugin.

I updated the parent version in Aviate, modified AviateActivator to print all properties grouped by source using the new method, installed the plugin using the KPM command, and verified that the expected properties were correctly retrieved.

Copy link
Member

@sbrossie sbrossie left a comment

Choose a reason for hiding this comment

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

Let's fix CI and check all tests pass prior we merge this PR though.

* Fixed bugs identified by SpotBugs Maven plugin.
Assert.assertTrue(f.delete(), "Unable to delete file " + f.getAbsolutePath());

final boolean deleted = f.delete();
Assert.assertTrue(deleted || !f.exists(), "Unable to delete file " + f.getAbsolutePath());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a safe check to gracefully handle scenarios where a temp file has been deleted by another part of the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since CI is green and PR is approved, merging the PR

@reshmabidikar reshmabidikar merged commit 6a10ede into killbill:master Jun 23, 2025
10 checks passed
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.

3 participants