Conversation
pyrit/cli/frontend_core.py
Outdated
| initializer_names: Optional[list[str]] = None, | ||
| env_files: Optional[list[Path]] = None, | ||
| log_level: str = "WARNING", | ||
| log_level: Optional[str] = None, |
There was a problem hiding this comment.
Sort of orthogonal to your changes but why isn't that an actual log level, as in the type?
There was a problem hiding this comment.
That's a good question. Should we keep it as a string or refactor to use the logging types? I'm not sure if there's a reason it's this way and not even something like Literal["WARNING", "INFO", ...]
There was a problem hiding this comment.
Technically they're numbers, see https://docs.python.org/3/library/logging.html#logging-levels
I would definitely suggest just using logging.INFO etc.
There was a problem hiding this comment.
Updated to use int in latest commit
pyrit/cli/frontend_core.py
Outdated
| self._initializer_names = initializer_names | ||
| self._env_files = env_files | ||
| self._log_level = validate_log_level(log_level=log_level) | ||
| from pyrit.setup import ConfigurationLoader |
There was a problem hiding this comment.
Should be in the latest commit
| Initialize PyRIT context. | ||
|
|
||
| Configuration is loaded in the following order (later values override earlier): | ||
| 1. Default config file (~/.pyrit/.pyrit_conf) if it exists |
There was a problem hiding this comment.
Make sure to update devcontainer configuration to get that file into the devcontainer (if it isn't already).
There was a problem hiding this comment.
It should be by default, but an explicit check has been added to the devcontainer shell script, as well as explicit creation of .pyrit_conf from .pyrit_conf_example.
|
|
||
| MYPY_CACHE="/workspace/.mypy_cache" | ||
| VIRTUAL_ENV="/opt/venv" | ||
| PYRIT_CONFIG_DIR="/home/vscode/.pyrit" |
There was a problem hiding this comment.
Should this be ~/.pyrit?
There was a problem hiding this comment.
In the devcontainer that is the home directory.
There was a problem hiding this comment.
Makes sense, but I guess wondering if using ~ is more generic for diff containers.
There was a problem hiding this comment.
In my experience this tends to fail given that ~ can alias to home for the root user during the build process. But I don't know enough about our Docker build pipeline to confirm it
| PYRIT_CONFIG_FILE="$PYRIT_CONFIG_DIR/.pyrit_conf" | ||
|
|
||
| # Create the .pyrit config directory and copy example config if not exists | ||
| if [ ! -d "$PYRIT_CONFIG_DIR" ]; then |
There was a problem hiding this comment.
it might be nice to have someone else weigh in on this. I'm hesitant about auto copying things over like this though. @romanlutz who is working on docker things
There was a problem hiding this comment.
I'm confused. What I would expect to happen is that my directory at ~/.pyrit shows up at ~/.pyrit inside the container which is /home/vscode/.pyrit.
What is the copying logic meant to copy? The example file? What would that be useful for?
There was a problem hiding this comment.
Right now it's also copying over the example if the file doesn't exist which probably isn't useful like you mention.
Would it make sense to copy over ~/.pyrit directory? Including the .env files there?
There was a problem hiding this comment.
What would be the most intuitive way to handle discovery and population of the config file in the Docker image? I'm not sure I understand Roman's original request to make sure the file is in the devcontainer, since if the example file is in the git repo then when the Docker image clones and builds it should be included. Is there something I'm missing?
There was a problem hiding this comment.
I feel like we have two main options:
- Leave
.pyrit_conf_exampleand ask users to create.pyrit_confusing it like we do with our.envfile(s) - Deliberately create a
.pyrit_conffile with certain values from the example as a template.
I'm in favor of the first given that it's more consistent with how we do environment variables but as I said I think I'm misunderstanding the premise
rlundeen2
left a comment
There was a problem hiding this comment.
My only concerns are I want someone else to look at the docker setup :)
Everything else looks good!
Description
Adds a config as
~/.pyrit_conf(~/.pyrit_conf_exampleincluded in repo) which can be loaded in by a newConfigurationLoaderto both make initializing pyrit less verbose and also parameterize pyrit the same way across the CLI and GUI. Configuration file support for the CLI tools enables more complex setups that were not possible or cumbersome previously. New features:initialize_pyrit_asyncmethod(s)Tests and Documentation
Added as
tests/unit/setup/test_configuration_loader.py.