Fix #263 (riak_core should not directly reference `riak-admin` commands in output) #401

Open
wants to merge 1 commit into
from

2 participants

@slfritchie

Stolen (poorly) from GNU gettext.

@slfritchie slfritchie Fix #263
1ec0743
@jrwest jrwest referenced this pull request in basho/riak_kv Oct 3, 2013
Open

Move core console commands to riak_core #684

@slfritchie

Looking for review: @engelsanchez @engelsanchez @jtuple @jrwest or other?

@jrwest

confirmed this is already on the kill bill marked for pending review. no one has claimed yet

@jrwest jrwest commented on the diff Dec 21, 2013
src/riak_core_console.erl
@@ -956,3 +957,57 @@ parse_cidr(CIDR) ->
[IP, Mask] = string:tokens(CIDR, "/"),
{ok, Addr} = inet_parse:address(IP),
{Addr, list_to_integer(Mask)}.
+
+%% For riak_core users outside of Basho's Riak KV application: If you
+%% define an OTP environment variable ExtraKey (see below) for the
+%% riak_core application, for example my_module, then any extra/custom
+%% text formatting text can be returned by calling the callback
+%% function my_module:extra_text/1 ... stolen (poorly) from GNU
+%% gettext.
+
+extra_text_cb(Label) ->
+ case app_helper:get_env(riak_kv) of
@jrwest
jrwest added a line comment Dec 21, 2013

we shouldn't be referencing riak_kv in riak_core. Besides tests the one place we do this is hashtree.erl and that is to support rolling upgrades from previous versions of riak where hashtree.erl was in riak_kv. It seems like this function should just look for a "registered" extra_text callback mod, which riak_kv_app or something should register.

@slfritchie
slfritchie added a line comment Dec 25, 2013

I don't understand "shouldn't be referencing riak_kv" ... there gotta be some way of figuring out if this riak_core app is bundled with riak_kv. I'm peeking at OTP env vars to see if we're bundled -- oughtta work inside or outside Riak, am I forgetting something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrwest

I'm ok w/ the general idea here but I think the callback can be simpler. The name of the -admin script is what we care about parameterizing over so why not just have applications indicate a script name, e.g. "riak-admin" or "my-admin" or whatever?

I suppose some applications could change the names of the -admin commands. For the sake of simplicity, however, I think it would be sufficient in a first pass to ignore this case. If we don't want to, then indicating the name of the -admin script and the name of the command being paired w/ the call in riak_core_console seems to cover all cases and is a bit more structured than the current approach in this PR.

Overall, I think we can probably get away w/ leaving this PR for a future cycle if there is not time to make changes. There are many other obstacles to writing riak_core apps outside of Riak that are larger than this one. Perhaps we can revisit this in a larger push to address those obstacles?

@jrwest

@slfritchie I didn't see any response to my comment above so I'm going to assume this isn't making it for 2.0. Did you want to leave it open to continue the discussion?

@slfritchie

This PR goes back to the refrigerator for pondering later....

@jrwest jrwest added this to the 2.1 milestone Mar 24, 2014
@jrwest

The refrigerator contains things in the 2.1 milestone. marked as such.

@jrwest

NOTE: original issue can be found at #263

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment