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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent logging from breaking special remotes #6675

Merged
merged 4 commits into from
May 6, 2022

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented May 3, 2022

This discontinues the value stdout as a a valid target for logging, since an exploration of #6486 showed, that there's no way to reliably make special remotes work with that setting or even just make them fail in a comprehensible way.

In addition there is a minor change in behavior of get_initialized_logger, since its argument logtarget was in fact ignored if datalad.log.target was set. That doesn't seem to make sense if the caller explicitly specifies a target.

Changelog

馃獡 Deprecations and removals

  • Discontinued the value stdout for use with the config variable datalad.log.target as its use would inevitably break special remote implementations.

馃彔 Internal

  • get_initialized_logger now lets a given logtarget take precendence over datalad.log.target.

@bpoldrack bpoldrack changed the title Use annexremote's LogHandler in special remotes Rectify logging from within special remotes May 3, 2022
@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #6675 (4bc3e3a) into master (9cec945) will increase coverage by 0.15%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #6675      +/-   ##
==========================================
+ Coverage   90.44%   90.59%   +0.15%     
==========================================
  Files         353      353              
  Lines       45672    46617     +945     
==========================================
+ Hits        41309    42234     +925     
- Misses       4363     4383      +20     
Impacted Files Coverage 螖
datalad/log.py 87.84% <85.71%> (-0.21%) 猬囷笍
datalad/local/tests/test_add_archive_content.py 99.73% <0.00%> (+0.11%) 猬嗭笍
datalad/support/tests/test_annexrepo.py 98.23% <0.00%> (+0.35%) 猬嗭笍
datalad/support/annexrepo.py 92.17% <0.00%> (+0.98%) 猬嗭笍

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 9cec945...4bc3e3a. Read the comment docs.

@bpoldrack bpoldrack force-pushed the sr-logging branch 3 times, most recently from 93159d2 to 232148b Compare May 4, 2022 11:35
There's no way to fully prevent logging to stdout from within special
remotes if this setting is allowed. (see dataladgh-6486)
This would break special remotes.
Previously, an explicitly given `logtarget` would be ignored if the
`datalad.log.target` config was set. This doesn't seem to make sense.
Instead, the config should be overruled when the call to this function
explicitly specifies a target.
@codeclimate
Copy link

codeclimate bot commented May 5, 2022

Code Climate has analyzed commit 4bc3e3a and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Security 2

View more on Code Climate.

@bpoldrack bpoldrack changed the title Rectify logging from within special remotes Prevent logging from breaking special remotes May 5, 2022
@bpoldrack bpoldrack added the semver-minor Increment the minor version when merged label May 5, 2022
@bpoldrack bpoldrack marked this pull request as ready for review May 5, 2022 08:50
@bpoldrack
Copy link
Member Author

FTR: No idea how code climate manages to complain about push here.

@bpoldrack bpoldrack merged commit 9c6b62c into datalad:master May 6, 2022
@mih mih mentioned this pull request Jul 1, 2022
40 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants