Skip to content
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

Merged
merged 3 commits into from
Mar 26, 2020

Conversation

alxbl
Copy link
Collaborator

@alxbl alxbl commented Mar 17, 2020

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 and player.ini which contains all configuration parameters for PyRDP. We could also theoretically have a single config.ini which all tools read, but that would increase log configuration complexity.

mitm.default.ini Outdated Show resolved Hide resolved
@alxbl alxbl marked this pull request as ready for review March 18, 2020 16:58
@alxbl alxbl added the enhancement New feature or request label Mar 19, 2020
@alxbl alxbl added this to the vNext milestone Mar 24, 2020
Copy link
Member

@obilodeau obilodeau left a 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 🤞

bin/pyrdp-player.py Outdated Show resolved Hide resolved
bin/pyrdp-player.py Outdated Show resolved Hide resolved
mitm.default.ini Outdated Show resolved Hide resolved
mitm.default.ini Outdated Show resolved Hide resolved
mitm.default.ini Outdated Show resolved Hide resolved
pyrdp/core/settings.py Outdated Show resolved Hide resolved
pyrdp/logging/log.py Show resolved Hide resolved
pyrdp/mitm/cli.py Show resolved Hide resolved
pyrdp/mitm/config.py Outdated Show resolved Hide resolved
pyrdp/player/config.py Outdated Show resolved Hide resolved
@obilodeau
Copy link
Member

There is a conflict to resolve before this can be merged too.

@obilodeau
Copy link
Member

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.

@obilodeau
Copy link
Member

Merge commit into integration with manual conflict resolution: 76b5b7a

@obilodeau
Copy link
Member

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 26, 2020

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.

@obilodeau
Copy link
Member

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...

@alxbl
Copy link
Collaborator Author

alxbl commented Mar 26, 2020

Done. I guess I'll expose the rest of the configuration sooner rather than later.

@obilodeau obilodeau merged commit 2de6fe4 into master Mar 26, 2020
@obilodeau
Copy link
Member

Time to rebase my friend! 🚀

@obilodeau obilodeau deleted the logging-configuration branch August 6, 2021 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants