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

Refactor config source to not keep in memory all the properties #218

Merged
merged 4 commits into from
Apr 4, 2024

Conversation

Sgitario
Copy link
Contributor

Before these changes, the Clowder config source was keeping in memory all the properties from all the sources in the existingValues property. Apart from this, the clowder config source was used even when the clowder file is not set.

This pull request addresses both issues by:

  • Refactoring the solution to use the ClowderPropertyHandler interface.

This interface implements a decorator pattern, so the config source will only keep the properties in memory if and only if there is any handler that handles it.

  • Also, if the clowder file does not exist, it will print a warning, and will not register the clowder config source.

Relates to SWATCH-2037

@Sgitario Sgitario requested a review from gwenneg January 31, 2024 14:08
@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-2037 branch from 6ad7543 to 5c0fbee Compare February 13, 2024 15:06
@gwenneg gwenneg self-assigned this Feb 29, 2024
@gwenneg
Copy link
Member

gwenneg commented Apr 2, 2024

We'll need an additional handler for the quarkus-unleash configuration.

@gwenneg
Copy link
Member

gwenneg commented Apr 3, 2024

@Sgitario ☝️

Sorry for the delay before this was reviewed.

@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-2037 branch from 5c0fbee to ce57b57 Compare April 3, 2024 13:24
@Sgitario Sgitario requested a review from gwenneg April 3, 2024 13:24
@Sgitario
Copy link
Contributor Author

Sgitario commented Apr 3, 2024

@Sgitario ☝️

Sorry for the delay before this was reviewed.

Many thanks @gwenneg . PR updated addressing your comments.

Sgitario and others added 4 commits April 4, 2024 09:35
…wderConfigSourceFactory.java

Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
…figSourceTest.java

Co-authored-by: Gwenneg Lepage <gwenneg@users.noreply.github.com>
@Sgitario Sgitario force-pushed the jcarvaja/SWATCH-2037 branch from 1887435 to 569481e Compare April 4, 2024 07:35
@Sgitario Sgitario requested a review from gwenneg April 4, 2024 07:36
@Sgitario
Copy link
Contributor Author

Sgitario commented Apr 4, 2024

@gwenneg PR updated with your latest comments.

Copy link
Member

@gwenneg gwenneg left a comment

Choose a reason for hiding this comment

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

LGTM

@gwenneg gwenneg merged commit 318e4b8 into RedHatInsights:main Apr 4, 2024
3 checks passed
@gwenneg
Copy link
Member

gwenneg commented Apr 4, 2024

I forgot that release-please-action requires a feat: or fix: prefix to trigger a new release 😅

It'd be easier if this project was released like the Quarkiverse extensions!

@Sgitario Sgitario deleted the jcarvaja/SWATCH-2037 branch April 4, 2024 07:49
@Sgitario
Copy link
Contributor Author

Sgitario commented Apr 4, 2024

I forgot that release-please-action requires a feat: or fix: prefix to trigger a new release 😅

ups, sorry. Will follow this pattern for the next time!

@gwenneg
Copy link
Member

gwenneg commented Apr 4, 2024

I just released 2.6.0. It'll be available in Maven Central soon-ish.

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.

2 participants