Hi Aleks, Thank you for sharing your thoughts!
I believe your ideas have merit, and many could positively impact the project. However, I don't think the implementation is as straightforward. If these configurations are managed externally, they need to be synced and updated across all running instances; otherwise, we risk inconsistencies between them. Additionally, the time required for synchronization across instances should be considered. Please don't misunderstand—I'm not a big fan of the current situation where we have: Global configurations in the database Configuration in application.properties Configuration in the tenant store database I definitely agree that the latter two should be merged, and we should move away from the third option. However, I’m still uncertain about the first one due to the challenges of synchronization and consistency across running instances. That said, I’m open to being convinced. In the long term, I agree that we should invest time and effort into implementing the changes you've suggested. For now, though, I was aiming to take a smaller step toward resolving a current issue that's causing problems and providing a better way to manage global configurations. Best regards, Adam > On 13 Sep 2024, at 18:57, Aleksandar Vidakovic <chee...@monkeysintown.com> > wrote: > > Hi Adam, > > ... my question would be: why do we need this service (read: rhetoric > question)? I know that the initial idea here was to introduce the capability > to change configuration without any downtime... but is it really that useful? > I think: > - it's confusing to have 2 different configuration concepts (actually 3 if we > consider the tenant configuration database)... there is > application.properties, the global configuration service... and the tenant > configuration > - nowadays there are better concepts to get the same functionality if not > better functionality (Kubernetes Config Maps, Spring configuration server...) > - changing configuration is/can be a delicate process... that's why the > Spring people reload the whole application context if the configuration > changed... for a reason I think > > Just imagine you are a devops person and want to set up Fineract for > production in a reproducible way... maybe with Terraform... Ansible... bash > scripts... doesn't really matter. Alright, so you take care of > application.properties (or the environment variables... boils down to pretty > much the same), but now suddenly you have to switch context from a file > (application.properties) to your database (and wait until it's available etc. > etc.) and additionally poke values into the database (plain sql statements? > with liquibase?...) to make your production environment work (I know that > people use the web UI to do this manually... well, I'd argue that that's not > devops). > > BTW: same argument applies to the tenant configuration DB... should also be > defined in application.properties > > In short: instead of dragging that global config service around, why not make > an effort and move configuration related stuff into a single source of truth, > application.properties and get rid of the service entirely? And while we are > at it we should do the same with tenant configuration (or at least offer it > as an optional way of running your Fineract). > > Cheers > > On Fri, Sep 13, 2024 at 6:16 PM Ádám Sághy <adamsa...@gmail.com > <mailto:adamsa...@gmail.com>> wrote: >> Dear Fineract Community, >> >> I would like to bring your attention to my recent PR: >> https://github.com/apache/fineract/pull/4057. I would greatly appreciate >> your feedback and thoughts on the matter. >> >> Historically, global configurations were introduced into the system via >> Flyway (up to version 1.6) and later through Liquibase scripts. In many >> cases, the ID field value was explicitly defined, effectively hardcoding it. >> >> Currently, the global configuration API provides the following capabilities: >> >> Fetching entries by ID >> Fetching entries by name (this functionality was previously introduced) >> Updating entries by ID only >> This PR adds the ability to update global configuration entries by name. >> >> >> Since the name field is unique for each global configuration, it serves as >> an ideal candidate for identifying entries. >> >> Additionally, this PR modifies the integration tests, changing the retrieval >> and updating of global configurations to rely solely on the entry name. >> >> >> This change improves readability and helps prevent issues caused by ID >> changes or inconsistencies due to custom configurations. >> >> Why should we prefer using the name instead of the ID? >> >> Hardcoded IDs in Liquibase scripts for global configurations can lead to >> conflicts and inconsistencies, especially when past entries have been >> deleted or when projects have introduced custom configurations. These issues >> may arise when new global configurations are added. >> I propose that, moving forward, we avoid providing hardcoded IDs for new >> global configurations. Instead, we should rely on the database’s sequence >> generator or identity solutions to assign IDs. This practice could also >> extend beyond global configurations, as hardcoded IDs are generally not >> recommended. >> I look forward to hearing your thoughts on this proposal. >> >> Best regards, >> Adam >>
