security/sssd2: Numerous cleanups #272
Conversation
|
Thanks @arrowd , I'll take a look at this soon. I've got some other sssd patches that are going in as well. Lots of folks finally starting to chime in ;-) |
|
In some cases I'm bumping into the following unimplemented function: https://github.com/SSSD/sssd/blob/a56b8d1aaf030fea196b65545dfe207ea10bdf50/src/util/find_uid.c#L328 I wonder if your WIP patches are going to cover that? |
|
I've implemented that missing function and will submit it after this PR gets in. Can you please take a look, @jhixson74 ? |
|
I am working on https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=279255 currently. It will get committed tomorrow. After that I can look at this. The patch in the bug implements the find uid function. |
|
@arrowd Would you mind opening this up on bugzilla with attached as patches? Currently I don't know how to do github merges. I'm not authorized and merging is blocked. I'll figure that out another day ;-) |
|
Actually, I can make the diff myself. Nevermind ;-) |
You can add github as a remote to your normal tree, the cherry-pick the changes on the pull request to main. It's super fast. Add the Pyll Request trailer and reviewed by and you're done. :) |
|
I use |
That works... my scripting does that... |
|
@arrowd Can you explain what changes you've made and why? Looking over them, I agree lots of cleanup can occur, but other things like changing the data directory I'm not going to do. |
|
The canonical data dir for P.S. You can comment on specific lines in the pull request to make the context of the question clearer. |
|
Restored one patch that was actually useful. |
|
@jhixson74 can we get this in? We're running sssd2 with this change at $WORK for months now. |
|
Rebased after @0mp change. |
|
I understand that PRs are not subject for maintainer timeouts, but maybe I just push this? |
|
I agree with @jhixson74 that it is a lot of changes that are not explained in the commit message. Also, it'd be easier to review if you separated the no-op cleanups that are no brainers to approve and commit from changes like the DATADIR. I created a Bugzilla PR so that it stays on the radar: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=280992 I work on sssd2 from time to time so I'll try to comment on the specific lines I find confusing next week or so. |
arrowd
force-pushed
the
sssd
branch
2 times, most recently
from
8e40c59
to
ff302fc
Compare
|
I split the change into multiple commits to ease reviewing. |
|
Yet another ping. |
|
Anything else I can do to get this in? |
|
Thank you for splitting the changes into smaller chunks. It's way easier to see what's going on there and will be easier to understand later on in the commit history. Assuming that you've tested the change and it builds in poudriere, the changes look alright. Reviewed by: 0mp Also, I think that at this point you can also go for |
|
I have not timed out. I have been watching this for a while. I do not agree with changing the path here. Many of the other changes are okay. If I am going to be bullied into forced changes, then by all means go ahead and just change the maintainer to yourself. Thanks. |
|
This is not constructive. How am I supposed to proceed after this comment?
Can you please name the commits you're OK with, so I can push them.
Where? If you're talking about changing P.S. You can leave review comments on specific lines in the "Files changed" tab to make the context clearer. |
I'm really sorry if the tone of my message turned out inappropriate. I assumed that the maintainer timeout is as a reasonable way forward as there were no comments on the issue for more than 2 weeks. The community appreciates your work, @jhixson74. I thought that you are out of time and wouldn't mind the fixes to land. From what I understand, the only patch that the current maintainer is not OK with is 7897c7d. I think that John has a good point in not liking the fixing of the DATADIR paths. Sure, it is not great at the moment and it would be nice to clean up the path names but I suspect that it may cause unnecessary problems for our users (no one likes the configuration paths to change in the name of a cleanup). From what I understand, the way forward would be to remove the controversial DATADIR cleanup for now. This would leave us with a general cleanup which boils down to a cleaner port and no changes to the users of binary packages. Would you be willing to review such a patch set, @jhixson74, if @arrowd prepared one? Thanks :) |
|
I am okay with all of the changes except the DATADIR. Thank you for summing things up @0mp. There are indeed users that would be very unhappy with this change.
I explained that here. Maybe I should've been more clear? |
Sorry, I can't find it. Can you post an URL?
I don't get it. These files are program's data, not configs. Users do not (well, should not) change any files under |
- Simplify depending on Kerberos - - Do not use gssapi:bootstrap - - Do not redefine variables already defined by USES - - Instead of patching, pass the KRB5_CONFIG env var - Trim unused dependencies - Remove hunks from the configure.ac patch that aren't needed anymore - Simplify SHEBANG_FILES - No need to define LIB_DIRS, DEBUG_FLAGS and STRIP - Bump PORTREVISION to catch possible regressions Tested by: arrowd Approved by: 0mp, jhixson Pull Request: #272 Sponsored by: Future Crew, LLC
Sponsored by: Future Crew, LLC
|
I have pushed all changes except the |
@jhixson74 Please take a look