Skip to content

Conversation

@wikumChamith
Copy link
Member

addressConfiguration.xml file has been renamed to addressConfiguration-core_demo.xml, causing an error when running the application:

`ERROR - AddressConfigurationLoader.loadAddressConfiguration(66) |2025-02-19T15:10:04,762| Address hierarchy configuration file appears invalid, skipping the loading process

This is caused by the address hierarchy module trying to find a file named addressConfiguration.xml. Renaming the file to its original name reverts it to its original state.

https://github.com/openmrs/openmrs-module-addresshierarchy/blob/f7a942394016253d85ab145d95170483e56465ba/api/src/main/java/org/openmrs/module/addresshierarchy/config/AddressConfigurationLoader.java#L33

https://openmrs.atlassian.net/browse/ADDR-138

openmrs/openmrs-module-addresshierarchy#45

Copy link
Contributor

@NethmiRodrigo NethmiRodrigo left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @wikumChamith!

@dkayiwa dkayiwa merged commit 29f481d into main Feb 21, 2025
1 check passed
@dkayiwa
Copy link
Member

dkayiwa commented Apr 2, 2025

This is not enough because content packages added referenceapplication-demo to the path.
The module looks for configuration/addresshierarchy/addressConfiguration.xml and yet content packages put it in configuration/addresshierarchy/referenceapplication-demo/addressConfiguration.xml. So we still have this error.

@dkayiwa
Copy link
Member

dkayiwa commented Apr 3, 2025

We also had to do a rename from addresshierarchy-core_demo.csv to addresshierarchy.csv

@ibacher
Copy link
Member

ibacher commented Apr 4, 2025

The module looks for configuration/addresshierarchy/addressConfiguration.xml

Ah... shoot. That means we have a domain that violates Iniz's normal rules (i.e., we can just put things in a subfolder). It'd be nice if we didn't have to hard-code a rule for this in the SDK, but maybe that's the right thing to do for now?

@mseaton
Copy link
Member

mseaton commented Apr 4, 2025

hard-code a rule for this in the SDK, but maybe that's the right thing to do for now?

Ick. I'd rather we fix this in Iniz. That said, it doesn't really make sense to support arbitrary addressHierarchy csv and/or xml files.

The SDK does support configuring whether files should be placed in a subdirectory named after the content package, or just put directly into the domain directory. Maybe we can make this configurable by domain in some way.

Personally, I think we should consider getting rid of the "content packages go in subdirectories" thing, in favor of promoting naming conventions of initializer files that reduce the likelihood of collisions.

At PIH we turn off the subdirectories thing and keep everything at a known, managed level.

@ibacher
Copy link
Member

ibacher commented Apr 4, 2025

The SDK does support configuring whether files should be placed in a subdirectory named after the content package

Yeah, but it would be nice not to require people to configure that.

Personally, I think we should consider getting rid of the "content packages go in subdirectories" thing

I get it, but I still think it's not a good idea to try and coordinate filenames across content packages developed by different organizations, which is the key thing that we want to enable for content packages. For use-cases like PIH where all of the content packages are built and maintained in-house this is less of an important consideration.

Definitely should fix this in Iniz. It looks very feasible to do so.

@ibacher
Copy link
Member

ibacher commented Apr 4, 2025

I was slightly worried that the AddressHierarchy module (which Iniz delegates to) was requiring them to be in a specific location, but that's not the case.

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.

6 participants