-
Notifications
You must be signed in to change notification settings - Fork 750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
security/sssd2: Numerous cleanups #272
base: main
Are you sure you want to change the base?
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. |
8e40c59
to
ff302fc
Compare
I split the change into multiple commits to ease reviewing. |
- Do not use gssapi:bootstrap - Do not redefine variables already defined by USES - Instead of patching, pass the KRB5_CONFIG env var Sponsored by: Future Crew, LLC
Sponsored by: Future Crew, LLC
…needed anymore Sponsored by: Future Crew, LLC
Sponsored by: Future Crew, LLC
Sponsored by: Future Crew, LLC
- Trim configure options - Remove unused variables - Fix DATADIR Sponsored by: Future Crew, LLC
Yet another ping. |
@jhixson74 Please take a look