-
Notifications
You must be signed in to change notification settings - Fork 242
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
feat: Add support for logging configuration files. #191
Conversation
f8f7075
to
e188747
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite a big change but it seems very well done. 🥇 💯 Only minor comments.
Also, I'm going to throw this into integration and see what our CI tells us 🤞
There is a conflict to resolve before this can be merged too. |
Hmm, the conflict is fairly involved because of the headless work and the fact that loggers got mass-renamed in the player. I would advise that the conflict resolution should be visible in the PR and not done during the merge. I think either a rebase or a merge from master in the branch will achieve that. |
Merge commit into integration with manual conflict resolution: 76b5b7a |
Integration passed CI: https://github.com/GoSecure/pyrdp/runs/534227539?check_suite_focus=true |
c352799
to
16b4696
Compare
I rebased on master, resolved conflicts and added deduplicated the configuration files. I'm going to fix the twistd CLI issue once and for all, as part of this, too. I don't want to document the logging configurations yet, as I'd like to expose other configuration settings in the .ini file before making it an official feature. |
3b78221
to
afbcfa3
Compare
I'm sorry to say it but this needs a mention in the README. The files are very self-documented so no need to copy content over but there should be a pointer to the existence of the ini files and where to grab them. Since our install workflow involves git this will be very simple. For malboxes, installed via pip+git, this is platform-dependent and complicated... |
Done. I guess I'll expose the rest of the configuration sooner rather than later. |
Time to rebase my friend! 🚀 |
This exposes all logging configuration (and eventually command line flags) through a configuration file.
The idea is to give users more control on what is output and eventually simplify launching the tool.
The PR is a draft because I want to clean up the existing logging code and add support for the player before merging.
The plan is to have a
mitm.ini
andplayer.ini
which contains all configuration parameters for PyRDP. We could also theoretically have a singleconfig.ini
which all tools read, but that would increase log configuration complexity.