Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
Hello Stephen,
thank you for your patch. I see little disruption of our coding style. My comments are inlined.
0001-DEBUG-Add-debug-alias-for-debug_level.patch
From f59256f027bb15a5cff317e5b1d418107b4a0a95 Mon Sep 17 00:00:00 2001 From: Stephen Gallagher sgallagh@redhat.com Date: Tue, 26 Apr 2016 11:04:36 -0400 Subject: [PATCH] DEBUG: Add `debug` alias for debug_level
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
Resolves: https://fedorahosted.org/sssd/ticket/2999
src/confdb/confdb.h | 1 + src/util/server.c | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index a9b1c4362b5c0c6b158830b1bf2ef68db09d8d06..153e68a0761463723945667004b4505acbc5a0b9 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -51,10 +51,11 @@
/* Services */ #define CONFDB_SERVICE_PATH_TMPL "config/%s" #define CONFDB_SERVICE_COMMAND "command" #define CONFDB_SERVICE_DEBUG_LEVEL "debug_level" +#define CONFDB_SERVICE_DEBUG_LEVEL_ALIAS "debug" #define CONFDB_SERVICE_DEBUG_TIMESTAMPS "debug_timestamps" #define CONFDB_SERVICE_DEBUG_MICROSECONDS "debug_microseconds" #define CONFDB_SERVICE_DEBUG_TO_FILES "debug_to_files" #define CONFDB_SERVICE_TIMEOUT "timeout" #define CONFDB_SERVICE_FORCE_TIMEOUT "force_timeout" diff --git a/src/util/server.c b/src/util/server.c index 67a25955783c30dc03f3d6d9193c8547c625f134..074dc34848ae2b8dd98bf6b218cc6c9c8441104d 100644 --- a/src/util/server.c +++ b/src/util/server.c @@ -565,18 +565,31 @@ int server_setup(const char *name, int flags,
if (debug_level == SSSDBG_UNRESOLVED) { /* set debug level if any in conf_entry */ ret = confdb_get_int(ctx->confdb_ctx, conf_entry, CONFDB_SERVICE_DEBUG_LEVEL,
SSSDBG_DEFAULT,
SSSDBG_UNRESOLVED, &debug_level); if (ret != EOK) { DEBUG(SSSDBG_FATAL_FAILURE, "Error reading from confdb (%d) " "[%s]\n", ret, strerror(ret));
^ please, remove this space
return ret; }
if (debug_level == SSSDBG_UNRESOLVED) {/* Check for the `debug` alias */ret = confdb_get_int(ctx->confdb_ctx, conf_entry,CONFDB_SERVICE_DEBUG_LEVEL_ALIAS,SSSDBG_DEFAULT,&debug_level);
^ this is the right indentation
if (ret != EOK) {DEBUG(SSSDBG_FATAL_FAILURE, "Error reading from confdb (%d) ""[%s]\n", ret, strerror(ret));return ret;}}debug_level = debug_convert_old_level(debug_level);}
/* same for debug timestamps */ if (debug_timestamps == SSSDBG_TIMESTAMP_UNRESOLVED) {
-- 2.7.4
The patch looks good to me. So, I will final ACK after: * CI (hash 0b9529e) * fixing of coding style.
Regards
On 04/27/2016 08:47 AM, Petr Cech wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
Hello Stephen,
thank you for your patch. I see little disruption of our coding style. My comments are inlined.
Hi Stephen,
I took the liberty and I fixed the nitpicks. Are you alright with attached patch?
[...]
The patch looks good to me. So, I will final ACK after:
- CI (hash 0b9529e)
CI passed http://sssd-ci.duckdns.org/logs/job/42/74/summary.html
ACKed by me.
Regards
On 04/27/2016 06:44 AM, Petr Cech wrote:
On 04/27/2016 08:47 AM, Petr Cech wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
Hello Stephen,
thank you for your patch. I see little disruption of our coding style. My comments are inlined.
Hi Stephen,
I took the liberty and I fixed the nitpicks. Are you alright with attached patch?
Looks fine to me. Thanks!
(It's been a long while and an editor-change since I last hacked on SSSD, so I guess my automatic formatter isn't quite right.)
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.
On 04/27/2016 05:57 AM, Pavel Březina wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.'
Yeah, I don't want this to become a common thing (we shouldn't really be aliasing anything), but this is such a *common* mistake that it's bordering on ridiculous not to just make an exception here.
When you get right down to it, most projects use the more abbreviated term "debug" anyway, so we're kind of the outlier.
(You will notice I intentionally didn't add it to the manual; this is meant to be a hidden convenience feature, not the primary method. Also, `debug_level` will always overrule `debug` if both are present.)
On (27/04/16 15:18), Stephen Gallagher wrote:
On 04/27/2016 05:57 AM, Pavel Březina wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.'
Yeah, I don't want this to become a common thing (we shouldn't really be aliasing anything), but this is such a *common* mistake that it's bordering on ridiculous not to just make an exception here.
When you get right down to it, most projects use the more abbreviated term "debug" anyway, so we're kind of the outlier.
(You will notice I intentionally didn't add it to the manual; this is meant to be a hidden convenience feature, not the primary method. Also, `debug_level` will always overrule `debug` if both are present.)
I don't prefer undocumeted options. If we really want to have an alias then it should be documented. Otherwise users might wonder why it magicaly works.
The option/alias will need to be added to the list of valid options anyway with the config validation (coming soon)
LS
On 04/28/2016 09:30 AM, Lukas Slebodnik wrote:
On (27/04/16 15:18), Stephen Gallagher wrote:
On 04/27/2016 05:57 AM, Pavel Březina wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.'
Yeah, I don't want this to become a common thing (we shouldn't really be aliasing anything), but this is such a *common* mistake that it's bordering on ridiculous not to just make an exception here.
When you get right down to it, most projects use the more abbreviated term "debug" anyway, so we're kind of the outlier.
(You will notice I intentionally didn't add it to the manual; this is meant to be a hidden convenience feature, not the primary method. Also, `debug_level` will always overrule `debug` if both are present.)
I don't prefer undocumeted options. If we really want to have an alias then it should be documented. Otherwise users might wonder why it magicaly works.
The option/alias will need to be added to the list of valid options anyway with the config validation (coming soon)
Ok, I added documentation and tests for it. See attached patch.
On Fri, Jun 03, 2016 at 09:34:46AM -0400, Stephen Gallagher wrote:
On 04/28/2016 09:30 AM, Lukas Slebodnik wrote:
On (27/04/16 15:18), Stephen Gallagher wrote:
On 04/27/2016 05:57 AM, Pavel Březina wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.'
Yeah, I don't want this to become a common thing (we shouldn't really be aliasing anything), but this is such a *common* mistake that it's bordering on ridiculous not to just make an exception here.
When you get right down to it, most projects use the more abbreviated term "debug" anyway, so we're kind of the outlier.
(You will notice I intentionally didn't add it to the manual; this is meant to be a hidden convenience feature, not the primary method. Also, `debug_level` will always overrule `debug` if both are present.)
I don't prefer undocumeted options. If we really want to have an alias then it should be documented. Otherwise users might wonder why it magicaly works.
The option/alias will need to be added to the list of valid options anyway with the config validation (coming soon)
Ok, I added documentation and tests for it. See attached patch.
I'm fine with the patch and since another user got confused about debug vs debug_level on #sssd recently, I would like to merge the patch. Petr already ack-ed the previous versions and I'm fine with the amendments.
Anyone against pushing the patch? If not, I'll push it..
On 06/07/2016 03:21 PM, Jakub Hrozek wrote:
On Fri, Jun 03, 2016 at 09:34:46AM -0400, Stephen Gallagher wrote:
On 04/28/2016 09:30 AM, Lukas Slebodnik wrote:
On (27/04/16 15:18), Stephen Gallagher wrote:
On 04/27/2016 05:57 AM, Pavel Březina wrote:
On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
Our users constantly make the mistake of typing `debug = 9` in the sssd.conf instead of `debug_level = 9` as would be correct. This happens frequently-enough that we should just alias it rather than continue to have people make mistakes.
I don't really oppose but I'd rather print a warning instead of aliasing it, otherwise we can end up aliasing everything. It may be done as part of configuration check patches that should detect typos.'
Yeah, I don't want this to become a common thing (we shouldn't really be aliasing anything), but this is such a *common* mistake that it's bordering on ridiculous not to just make an exception here.
When you get right down to it, most projects use the more abbreviated term "debug" anyway, so we're kind of the outlier.
(You will notice I intentionally didn't add it to the manual; this is meant to be a hidden convenience feature, not the primary method. Also, `debug_level` will always overrule `debug` if both are present.)
I don't prefer undocumeted options. If we really want to have an alias then it should be documented. Otherwise users might wonder why it magicaly works.
The option/alias will need to be added to the list of valid options anyway with the config validation (coming soon)
Ok, I added documentation and tests for it. See attached patch.
I'm fine with the patch and since another user got confused about debug vs debug_level on #sssd recently, I would like to merge the patch. Petr already ack-ed the previous versions and I'm fine with the amendments.
Anyone against pushing the patch? If not, I'll push it..
Ack from me as well.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Březina -
Petr Cech -
Stephen Gallagher