erlware / relx

Sane, simple release creation for Erlang
http://erlware.github.io/relx
Apache License 2.0
696 stars 233 forks source link

Allow applications to provide alternative user-facing messages #918

Open martinsumner opened 2 years ago

martinsumner commented 2 years ago

We're using relx for the latest release of the Riak database, and it would be helpful to change some of the user-facing messages that are produced by the extended start script.

The primary issue is the deprecation warning on start. This makes sense to us as relx users (and riak developers), but for a Riak user/operator that is abstracted from the existence of relx/rebar3 being pointed to rebar3.org is confusing. There are also some legitimate use cases for using start with Riak.

I've hacked together a clumsy change whereby the deprecation warning is deferred until after the pre_start hooks are run, and defined as an environment variable so that we can export an alternative text in the pre-start hooks:

https://github.com/martinsumner/relx/pull/1/files

I'm happy to work on an alternative approach if there is a better answer, or one that allows for overriding messages in a more generic way (perhaps also tackling https://github.com/erlware/relx/issues/758 ?)

hmmr commented 2 years ago

To elaborate, while the suggestion to start riak (or any other Erlang app built with rebar3 release, for that matter) as a properly registered systemd service is generally right and proper, it's only valid in production setting. During testing, we need to be able to start and stop riak by means of direct ./rel/riak/bin/riak start, in which case we (1) don't bother with a special user or root, (2) keep all artifacts and logs entirely under rel/riak, (3) have no dealings with systemd. There is nothing wrong with this practice, and we would like to keep it; rebar-generated releases should remain orthogonal to any distro-specific way of starting/stopping the erlang applications in those releases as systemd services.

ferd commented 2 years ago

Is the problem that you really want the command to be named 'start' and not 'daemon'?

The command is going to be removed at some point -- it was removed because of that confusion but then added back with the message as a compromise -- mostly because people use it all the time even in contexts where it wouldn't make sense and we hoped to eventually switch to actually descriptive names.

Or is there a risk or concern that you can't change that value and you're hoping it is supported as-is forever?

hmmr commented 2 years ago

No objections to start renamed as daemon as such. But it would be desirable to continue to have a command with this behaviour.

ferd commented 2 years ago

yeah the long term behaviour is expected to stay as 'daemon', it's just 'start' itself that was counterintuitive because they're all 'start' related but it was not descriptive of its role: foreground, console, daemon -- vs the old style foreground, console, start.

If you switch to daemon mode I believe you'll have the same exact behavior without the warning. It also makes it easier to know how to connect to a node (remote_console for console, daemon_attach for daemon). It was really confusing for people to figure out that remote_console does not work with start, only daemon_attach does, because the starting mode has that impact.

tsloughter commented 2 years ago

So, to be clear, users run the start command manually themselves? And the issue is not wanting them to switch to having to call daemon instead? I'd much rather remove the message by removing the start command ;)

martinsumner commented 2 years ago

No.

Users currently use start. We're happy that they will need to switch to using daemon.

However, in the meantime there is a deprecation notice, and we would prefer to be able to override the text for Riak users, with a text that is more directly associated with our use case. Hence the proposal for a way that an application could write its own deprecation text via a pre_start hook.

martinsumner commented 2 years ago

There is also a more general question. Perhaps there are other examples of user-facing text (e.g. the response on success/failure to ping) where an application using relx may have a different idea of a what text would be suitable to their users. Perhaps there is a general need to define text that will be presented to the user, in such a way that an application using relx could always override with its own wording.

I'm happy to help work on this - with either a specific fix to allow override of the deprecation notice to an alternative text, or a more general change. But I'm interested to see if you consider this to be a useful or acceptable change.

aef- commented 1 year ago

Hey all -- just bumping this to see if we can get Martin's proposal merged in.

tsloughter commented 1 year ago

@aef- that is probably fine. A more general solution would be nice but I think this would be acceptable for the time being.

martinsumner commented 1 year ago

Thanks @tsloughter. Currently I've updated the deprecation message on start by allowing the message to be over-written in pre-start hook. I was considering what an effective more general solution should be.

Would it be better if I was to add a new type of hook script - message_override - and everywhere in extended_bin where a message is returned to the user, the message_override hooks scripts would be called first allowing the environment variables for messages to be updated. Would that be a preferable PR, or is this too much additional change and complexity?

If there's an easy way of making it more general, I'm happy to make this part of the PR.

In our relx branch we also have an update to allow use_nodetool to be set directly in rebar.config (https://github.com/basho/riak/blob/develop/rebar.config#L72). Are you happy with this extension, and if so would you prefer this to be separated into a different PR to the message override?