Log to stderr by default instead of salt-server.log#213
Log to stderr by default instead of salt-server.log#213koppchen wants to merge 2 commits intodcermak:mainfrom
Conversation
Update the default value of the "--log-level" flag to "warning".
Output logs to stderr by default instead of salt-server.log, to stop the server from creating log files in the current working directory. Add a "--log-file" flag to support writing logs to a given file instead of stderr. Update SaltServer and SlsFileWorkspace so they use the "--log-level" flag value.
koppchen
force-pushed
the
logging-updates
branch
from
336590f to
d49df73
Compare
|
Hmm, I'm not sure why the checks on this PR never ran. Checking the workflows, it looks like they should run for all pull requests, but maybe I'm missing something... I did test the ci.yml and vscode_extension.yml workflows on a separate branch and only the |
|
This completely fell under my radar, sorry. Could you try to rebase your commits on main (just so that they get a newer timestamp) and force push? |
koppchen
force-pushed
the
logging-updates
branch
from
d49df73 to
336590f
Compare
|
No worries. Looks like that worked, and is showing "This workflow is awaiting approval from a maintainer" now, so that's good. (there's no rush at all) Let me know if there's any problems, concerns, or changes needed with this change. Also feel free to reject it if it seems like it could be problematic. (I've been using it with neovim for a few months now without issues) Thank you! |
| log_level = loglevel_from_str(args.log_level[0]) | ||
| logging.basicConfig( | ||
| filename="salt-server.log", | ||
| filename=args.log_file, |
There was a problem hiding this comment.
Shouldn't this be?
| filename=args.log_file, | |
| filename=args.log_file[0], |
There was a problem hiding this comment.
The [0] would be needed if nargs=1 is used. (as mentioned in the other comment)
Oh, btw, I am doing something with this that might be a little confusing. If --log-file isn't passed in as an arg, then args.log_file defaults to None, and that's still passed to basicConfig(). However, basicConfig sets filename to None by default if it's not specified, which is why this works fine.
The related basicConfig() code is here:
https://github.com/python/cpython/blob/main/Lib/logging/__init__.py#L2118-L2129
| parser.add_argument( | ||
| "--log-file", | ||
| help="Redirect logs to the given file instead of writing to stderr", | ||
| ) |
There was a problem hiding this comment.
| parser.add_argument( | |
| "--log-file", | |
| help="Redirect logs to the given file instead of writing to stderr", | |
| ) | |
| parser.add_argument( | |
| "--log-file", | |
| help="Redirect logs to the given file instead of writing to stderr", | |
| nargs=1, | |
| type=str, | |
| ) |
There was a problem hiding this comment.
type: Hmm checking the docs for type, it looks like it defaults to string, which agrees with what I see while testing. So we should be ok there, however we can add it to be more explicit if we want?
From https://docs.python.org/3/library/argparse.html#type
By default, the parser reads command-line arguments in as simple strings. However, quite often the command-line string should instead be interpreted as another type, such as a float or int.
nargs: Ahh, I recall having trouble with nargs in the past since it can be a little confusing. Looks like using nargs=1 would convert it to a list, which would require the [0] usage later on.
From https://docs.python.org/3/library/argparse.html#nargs
The supported values are:
- N (an integer). N arguments from the command line will be gathered together into a list.
...
Note that nargs=1 produces a list of one item. This is different from the default, in which the item is produced by itself.
It might be more clear to leave the nargs=1 off, so we don't need the [0] later?
Would you be willing to share your neovim config as well? We have client code for VSCode & Emacs in the repo, but nothing for neovim yet. |
|
neovim config: Sure. My current config contains a large number of un-related plugins/configs, consists of 12 files, and has limited (non-lsp) vim support. Let me know if it would be helpful to see all that? (I'd be happy to share it, if it's helpful) Otherwise, I'm looking into stripping out un-related items, to simplify the config for this, since that might be more helpful. But it would still use at least 3 other plugins, which make installing this & other lsp code easy: |
2 participants
Output logs to stderr by default instead of salt-server.log, to stop the
server from creating log files in the project root directory or current
working directory.
Add a "--log-file" flag to support writing logs to a given file instead
of stderr.
Update SaltServer and SlsFileWorkspace so they use the "--log-level"
flag value.
Update the default value of the "--log-level" flag to "warning".
Using this with neovim leaves
salt-server.logfiles around. This change stops that, but still allows the user to specify a log file with--log-file, if needed. Tests pass withpytest. This seems to be working fine in neovim, however I'm not sure if logging to stderr will cause any issues with emacs or vscode.