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

ConfigManager can be limited to local config; prevent remote changes of hooks #3907

Merged
merged 5 commits into from Dec 9, 2019

Conversation

mih
Copy link
Member

@mih mih commented Dec 9, 2019

The issue was raised in #3903 (comment) but is in fact a long-standing issue in DataLad. Hence this comes as a separate PR, and #3903 will need to be adjusted, once this is merged.

We may want to have an override that re-enables this feature -- but I think we should wait if an actual use case materializes. Otherwise not supporting it seems like the better way.

mih added 3 commits Dec 9, 2019
i.e. to not read any configuration that is commited in a dataset.
This is a needed mode of operation in any situation where it is
not safe to have configuration pull from elsewhere affect local
operation, e.g. the definition and execution of hooks (see
datalad#3903)
@mih mih mentioned this pull request Dec 9, 2019
2 tasks
@codecov
Copy link

@codecov codecov bot commented Dec 9, 2019

Codecov Report

Merging #3907 into master will increase coverage by 9.7%.
The diff coverage is 90.32%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #3907     +/-   ##
=========================================
+ Coverage   71.01%   80.71%   +9.7%     
=========================================
  Files         272      272             
  Lines       36065    36077     +12     
=========================================
+ Hits        25611    29121   +3510     
+ Misses      10454     6956   -3498
Impacted Files Coverage Δ
datalad/interface/tests/test_run_procedure.py 100% <ø> (ø) ⬆️
datalad/support/gitrepo.py 82.99% <100%> (+32.28%) ⬆️
datalad/tests/test_config.py 100% <100%> (+1.19%) ⬆️
datalad/interface/utils.py 88.37% <100%> (+18.72%) ⬆️
datalad/config.py 97.77% <83.33%> (+21.53%) ⬆️
datalad/downloaders/tests/test_http.py 58.08% <0%> (-2.21%) ⬇️
datalad/support/tests/test_gitrepo.py 99.89% <0%> (+0.1%) ⬆️
datalad/distribution/tests/test_install.py 99.79% <0%> (+0.2%) ⬆️
datalad/interface/tests/test_rerun.py 100% <0%> (+0.4%) ⬆️
datalad/log.py 65.86% <0%> (+0.48%) ⬆️
... and 74 more

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 6031944...0c2b431. Read the comment docs.

@bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented Dec 9, 2019

We may want to have an override that re-enables this feature -- but I think we should wait if an actual use case materializes. Otherwise not supporting it seems like the better way.

Agree. We also may want to have a more fine-grained restriction that cares about execution related configs only. But again: Having this as is in this PR should give us a basis to discover what else we might need to consider. Let's have that and see what emerges from that.

@mih
Copy link
Member Author

@mih mih commented Dec 9, 2019

Thanks I will merge this, once travis is done.

kyleam
kyleam approved these changes Dec 9, 2019
Copy link
Contributor

@kyleam kyleam left a comment

The changes look good to me, and like @bpoldrack I agree with the rationale of not providing a way to restore the disabled behavior until the need arises.

@mih
Copy link
Member Author

@mih mih commented Dec 9, 2019

Perfect, thanks to both of you!

@mih mih merged commit b45ca69 into datalad:master Dec 9, 2019
17 checks passed
@mih mih deleted the enh-localconfig branch Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants