-
Notifications
You must be signed in to change notification settings - Fork 77
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
Configuration interface is confusing and poorly documented #176
Comments
Have you read the documentation found here: https://github.com/eiffel-community/eiffel-remrem-publish/blob/master/wiki/markdown/configuration.md ? I don't know if it helps in your particular case but I agree it is confusing if the documentation on Github pages and the one in the source code repository differ. I have raised an issue about the documentation being spread out and not easily found, previously in #168 . |
Not sure if I read that page. It lists the jsonlist property and gives a few examples but doesn't document the semantics of the keys in the JSON objects. A documentation cleanup and expansion would help, but a code cleanup would make both the code and the documentation easier to maintain. |
We have an issue to move away from gh-pages documentation towards master branch.
|
Yes, but rewriting the documentation won't fix the fact that the code
If you don't agree that this is something we should address independently of the documentation feel free to close the issue and focus on #168. I can help out writing the new prose or review PRs. |
Description
I find the configuration interface of REMReM Publish confusing, inconsistent, and poorly documented with too many partially overlapping ways of doing things. I've had to read the code back and forth to understand what's going on. This is related to #168 but runs deeper.
Background
If you look at the example property file there's the
rabbitmq.instance.jsonlist
property that you can populate with a list of message protocols and associated RabbitMQ configurations. Absent was an option for the virtualhost which for me ruled out using our production RabbitMQ cluster (I filed a separate issue for this; #175).I switched to using a local instance, but I didn't want to configure TLS for it. I couldn't find any documentation for the
tls
key except what's said about the<protocol>.rabbitmq.tls
property on https://eiffel-community.github.io/eiffel-remrem-publish/serviceUsage.html, which states that it's optional. That's not true for thetls
key in the jsonlist property; leaving it out results in an NPE. After reading the code I found I could set it to an empty string. I also learned that if I don't care about the exact TLS version I can settls
to any string that contains "default" and it'll just enable TLS without any version requirements.On https://eiffel-community.github.io/eiffel-remrem-publish/serviceUsage.html I also found the "Exchange configurations" section which details another set of properties that can be set to configure RabbitMQ, but these properties are only read if you've also specified the
rabbitmq.instance.jsonlist
property so I don't understand why they even exist, never mind why they're the only documented configuration interface.While reading the code I also found that the CLI uses an entirely different set of properties, and that it's only there you can configure whether to publish persistent or transient messages. Luckily persistent is the default.
Proposed Actions
Unless there are really compelling reasons for using properties for configuration—I have limited Java skills so there might be cleverness I'm not aware of—drop it and use a regular configuration file that works well for hierarchical data structures like lists of RabbitMQ configurations. YAML works fine (even as Spring Boot property files, it seems) and should be possible to deserialize straight into Java objects. Remove support for the CLI-only properties.
Motivation
I'm working on adopting Eiffel and have spent a couple of hours just getting REMReM Publish to fire up. I've abandoned open source projects over less friction than this. Since it has dependencies to other services (unlike REMReM Generate) some configuration is clearly necessary but it shouldn't be this hard.
Possible Drawbacks
Fixing the root cause will require changes that aren't backwards compatible.
The text was updated successfully, but these errors were encountered: