8:05 a.m.
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: opened
PR body:
"""
Move `sss_cache` into a new command **sssctl expire-cache** and `sss_debuglevel` into new
command **sssctl debug-level** for ticket https://pagure.io/SSSD/sssd/issue/3057
Shell redirect wrappers replace the `sss_{debuglevel,cache}` binaries and man pages
updated to point admins towards the new **sssctl** commands.
"""
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
8:06 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
centos-ci commented:
"""
Can one of the admins verify this patch?
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-301468941
8:06 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
centos-ci commented:
"""
Can one of the admins verify this patch?
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-301468954
8:11 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
NACK to sss_cache changes.
sss_cache is part of sssd-common but sssctl is part of sssd-tools.
So after this change shell wrapper `sss_cache` needn't work in some cases due to
missing sssctl
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-301470402
10:07 a.m.
New subject: [sssd PR#274][+Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: +Changes requested
12:52 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
@lslebodn Yes, you make a good point. Is there a suggested way to handle this or do you
prefer to drop the `sss_cache` commits altogether?
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-302769194
1:11 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
jhrozek commented:
"""
I still see value in having sssctl manage the cache expiration because especially for sudo
rules, we not only need to expire the rules in the cache, but also tell the deamon to
fetch the rules again. There I was thinking of having an interface provided by the IFP
responder that the tool could call into. And having a reliable way of expiring sudo rules
would be handy, it's something I see mentioned in support cases a log.
@lslebodn will probably have a better idea than I how to solve the packaging problem but
what about just moving the sss_cache wrapper to sssd-tools? Of course, upgrades will then
be problematic, because if someone doesn't have sss-tools installed, then sss_cache
would be 'lost' for this user on upgrade. But I'm not sure I have a better
idea short of forcing sss-tools on everyone who had sssd-common previously..
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-302773603
11:38 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
I am not sure what is the least intrusive way to handle this but moving the `sss_cache`
wrapper to `sssd-tools` makes sense to me. Also, I can add some information to the wrapper
to cover the 'sss_cache lost during upgrade' case:
```
+#!/bin/sh
+sbindir=@sbindir@
+echo "Due to packaging changes, please make sure the sssd-tools package is
installed"
+echo "Redirecting to $sbindir/sssctl expire-cache"
+$sbindir/sssctl expire-cache $@
```
@lslebodn what do you think about it?
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-305550258
11:45 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
Extra message is helpful for users but it still will not fix tests or any other automated
script.
In theory we could merge the package `sssd-tools` into `sssd-common`. But it pull
additional dependencies:
```
sh# dnf install --assumeno /usr/sbin/sssctl
Last metadata expiration check: 0:01:15 ago on Wed Jul 19 16:34:23 2017.
Dependencies resolved.
================================================================================
Package Arch Version Repository Size
================================================================================
Installing:
sssd-tools x86_64 1.15.2-5.fc26 fedora 357 k
Installing dependencies:
libsss_simpleifp x86_64 1.15.2-5.fc26 fedora 70 k
python3-sss x86_64 1.15.2-5.fc26 fedora 90 k
sssd-dbus x86_64 1.15.2-5.fc26 fedora 164 k
Transaction Summary
================================================================================
Install 4 Packages
Total download size: 681 k
Installed size: 1.4 M
Operation aborted.
```
And I would like to do opposite. Reduce `sssd-common` due to `sssd-kcm` and
`sssd-secrets`.
ATM the simplest would be to drop removing `sss_cache`. But we can still have "an
allias" `sssctl expire-cache` -> `sss_cache`. Because I like name `expire_cache`
more then `sss_cache` :-)
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-316446652
5:22 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
mzidek-rh commented:
"""
In sssctl we follow (or at least try to) the TOPIC-ACTION convention for naming the
commands. So "expire-cache" is not a good name. It should be
'cache-expire'. We already have 'cache' topic in the local data section
and I think this command fits there to.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-316662360
5:46 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
On (20/07/17 10:22), mzidek-rh wrote:
In sssctl we follow (or at least try to) the TOPIC-ACTION convention
for naming the commands. So "expire-cache" is not a good name. It should be
'cache-expire'. We already have 'cache' topic in the local data section
and I think this command fits there to.
That's true but as I mentioned in previous comment.
The simplest would be to drop removing `sss_cache`
Ideal would be to have hybrid way how to call subcommands.
git has binaries in /usr/libexec/git-core/ or other paths.
In such way, we could have /usr/bin/sssctl-cache-expire which would have
minimal dependencies and could be easily called from shell wrapper
/usr/bin/sss_cache because it would be still part of package sssd-common
even though package sssd-tools will not be installed.
But that's probably a little bit bigger scope for task which
Justin can do as part of his job :-)
LS
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-316667054
2:58 p.m.
New subject: [sssd PR#274][synchronized] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
3:03 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
PR updated:
- Dropped the sss_cache commits
- Rebased to master
- Added new commit adding `sssctl cache-expire` command as a simple wrapper for
`sss_cache`
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-317855700
3:10 a.m.
New subject: [sssd PR#274][-Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: -Changes requested
6:46 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
mzidek-rh commented:
"""
There is a tabulator instead of space after '=' sign
in Makefile.am:
```
458
459 dist_sbin_SCRIPTS = contrib/tools/sss_debuglevel
460
```
But as Lukas mentioned we should not put sss_debuglevel in the tarball directly,
but only the .in version and call the replace_script on it in the Makefile.am,
so that proper paths are expanded in the script (see how replace_script is used
in the Makefile.am for other files). So the configure.ac line should be removed
too.
Also I would not put this to the contrib directory because this is directory
that we use for things that are not packaged in the tarball or nice things that
someone creted but are not core parts of the project (like some helpful sripts
etc). Maybe a new directory src/tools/wrappers would be more appropriate?
@lslebodn do you have any proposal in this regard?
I can see these warnings when I run autoreconf -if indicating you forgot to remove
some lines from Makefile.am:
```
Makefile.am: installing 'build/depcomp'
Makefile.am:1685: warning: variable 'sss_debuglevel_SOURCES' is defined but no
program or
Makefile.am:1685: library has 'sss_debuglevel' as canonical name (possible typo)
Makefile.am:1688: warning: variable 'sss_debuglevel_LDADD' is defined but no
program or
Makefile.am:1688: library has 'sss_debuglevel' as canonical name (possible typo)
```
Bad indentation in src/tools/sssctl/sssctl.h
```
113 errno_t sssctl_debug_level(struct sss_cmdline *cmdline,
114 struct sss_tool_ctx *tool_ctx,
115 void *pvt);
^ bad indentation
```
In src/tools/sssctl/sssctl_logs.c, the config.h should be the first header that
is included (I think we do not follow this everywhere, but still..)
Another bad indentation in src/tools/sssctl/sssctl.h
```
101 errno_t sssctl_cache_expire(struct sss_cmdline *cmdline,
102 struct sss_tool_ctx *tool_ctx,
103 void *pvt);
104 ^^^ bad indentation
```
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-318339105
8:55 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
Thank you for the review @mzidek-rh - I will make the changes and update the PR.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-318369074
10:08 p.m.
New subject: [sssd PR#274][synchronized] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
10:11 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
Patches updated, I believe the Makefile changes are done as Michal proposed but please let
me know if anything needs correcting.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-318799941
4:47 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
jhrozek commented:
"""
I'm sorry this PR review stalled (again :-/) but I've read the patches and I like
them now. I have two nitpicks, one was found by Coverity:
````
Error: CHECKED_RETURN (CWE-252):
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:307: check_return: Calling
"sssctl_run_command" without checking return value (as is done elsewhere 7 out
of 8 times).
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:108: example_assign: Example 1: Assigning:
"ret" = return value from "sssctl_run_command("sss_override
user-export /var/lib/sss/backup/sssd_user_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:110: example_checked: Example 1 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:332: example_assign: Example 2: Assigning:
"ret" = return value from "sssctl_run_command(cmd)".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:333: example_checked: Example 2 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:161: example_assign: Example 3: Assigning:
"ret" = return value from "sssctl_run_command("sss_override
user-import /var/lib/sss/backup/sssd_user_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:163: example_checked: Example 3 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:170: example_assign: Example 4: Assigning:
"ret" = return value from "sssctl_run_command("sss_override
group-import /var/lib/sss/backup/sssd_group_overrides.bak")".
sssd-1.15.4/src/tools/sssctl/sssctl_data.c:172: example_checked: Example 4 (cont.):
"ret" has its value checked in "ret != 0".
sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:292: example_assign: Example 5: Assigning:
"ret" = return value from "sssctl_run_command(cmd)".
sssd-1.15.4/src/tools/sssctl/sssctl_logs.c:293: example_checked: Example 5 (cont.):
"ret" has its value checked in "ret != 0".
# 305| fprintf(stderr, _("Missing option. \n"));
# 306| cachecmd = "sss_cache --help";
# 307|-> sssctl_run_command(cachecmd);
# 308| ret = EXIT_FAILURE;
# 309| goto done;
````
And the other I'll point out inline.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-329431797
4:50 a.m.
New subject: [sssd PR#274][+Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: +Changes requested
5:44 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
jhrozek commented:
"""
In addition, distcheck failed:
```
ERROR: files left in build directory after distclean:
./src/tools/wrappers/sss_debuglevel
make[1]: *** [distcleancheck] Error 1
```
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-329444877
10:02 p.m.
New subject: [sssd PR#274][synchronized] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
10:04 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
Thank you for the review @jhrozek
Patchset updated.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-330415031
5:31 a.m.
New subject: [sssd PR#274][-Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: -Changes requested
7:06 a.m.
New subject: [sssd PR#274][+Accepted] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: +Accepted
7:06 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
jhrozek commented:
"""
CI pased: http://vm-058-233.XXX/logs/job/77/62/summary.html
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-330518395
7:07 a.m.
New subject: [sssd PR#274][-Accepted] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: -Accepted
7:08 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
few nitpicks: 1st patch "SSSCTL: Replace sss_debuglevel with shell wrapper"
should be after introducing command "sssctl debug*" otherwise it will break
bisect an may be tests
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-330518891
7:53 a.m.
New subject: [sssd PR#274][synchronized] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
7:54 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
Thanks for the comments @lslebodn, changes made.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-330529794
10:10 a.m.
New subject: [sssd PR#274][+Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: +Changes requested
11:36 a.m.
New subject: [sssd PR#274][synchronized] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: synchronized
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
11:40 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
@lslebodn comments addressed, I was primarily following similar sssctl functions as an
example.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331213596
2:17 p.m.
New subject: [sssd PR#274][-Changes requested] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: -Changes requested
9:46 a.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
@justin-stephenson, I did few changes to build each commit separately (make git bisect
happy :-)
Are you fine with them?
https://pagure.io/fork/lslebodn/SSSD/sssd/commits/pr274_changes
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331904859
12:41 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
justin-stephenson commented:
"""
@lslebodn yes looks good to me, thank you for that.
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331957533
2:52 p.m.
New subject: [sssd PR#274][closed] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Author: justin-stephenson
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Action: closed
To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/274/head:pr274
git checkout pr274
2:52 p.m.
New subject: [sssd PR#274][comment] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
lslebodn commented:
"""
master:
* f74408e37a3007aa41b19ab2afb693a91694da42
* da19eaea902744ec3cb41f87fa93fadb767f90e7
* d2c614143870e6efd4b3ab20c3a55cf714595256
"""
See the full comment at https://github.com/SSSD/sssd/pull/274#issuecomment-331992958
2:52 p.m.
New subject: [sssd PR#274][+Pushed] Merge sss_cache and sss_debuglevel into sssctl
URL: https://github.com/SSSD/sssd/pull/274
Title: #274: Merge sss_cache and sss_debuglevel into sssctl
Label: +Pushed
2405
days inactive
2538
days old
sssd-devel@lists.fedorahosted.org
38 comments
5 participants
participants (5)
-
centos-ci -
jhrozek
-
justin-stephenson
-
lslebodn
-
mzidek-rh