search

eiffel-community / eiffel-remrem-publish

eiffel-remrem-publish
https://eiffel-community.github.io/eiffel-remrem-publish
Apache License 2.0
8 stars 77 forks source link

Configuration interface is confusing and poorly documented #176

Open magnusbaeck opened 5 years ago

magnusbaeck commented 5 years ago

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 the tls 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 set tls 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.

e-pettersson-ericsson commented 5 years ago

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 .

magnusbaeck commented 5 years ago

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.

SantoshNC68 commented 4 years ago

We have an issue to move away from gh-pages documentation towards master branch. There we will have a more readable documentation available. May be we will add them as :

magnusbaeck commented 4 years ago

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.