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

Fix post-clone reconfiguration for ORA remote #5672

Merged
merged 2 commits into from
May 19, 2021
Merged

Conversation

bpoldrack
Copy link
Member

@bpoldrack bpoldrack commented May 19, 2021

See #5628. We did not consider the recently introduced push-url of ORA
special remotes for matching against the clone URL. With this patch we
stick to the logic that tries a reconfiguration if either all known ORA
remotes failed to autoenable or none of the enabled ORA remotes have a
matching URL on record.

However, as pointed out in #5628, this only is a quick fix for a
particular usage pattern. A general solution for the reconfiguration
mess appears to be to switch to local configs only.

  • add test

See datalad#5628. We did not consider the recently introduced `push-url` of ORA
special remotes for matching against the clone URL. With this patch we
stick to the logic that tries a reconfiguration if either all known ORA
remotes failed to autoenable or none of the enabled ORA remotes have a
matching URL on record.

However, as pointed out in datalad#5628, this only is a quick fix for a
particular usage pattern. A general solution for the reconfiguration
mess appears to be to switch to local configs only.
@codecov
Copy link

codecov bot commented May 19, 2021

Codecov Report

Merging #5672 (a1107ef) into master (2fd46ee) will decrease coverage by 20.65%.
The diff coverage is 100.00%.

❗ Current head a1107ef differs from pull request most recent head e2330fc. Consider uploading reports for the commit e2330fc to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5672       +/-   ##
===========================================
- Coverage   90.47%   69.82%   -20.66%     
===========================================
  Files         305      302        -3     
  Lines       41850    41847        -3     
===========================================
- Hits        37865    29219     -8646     
- Misses       3985    12628     +8643     
Impacted Files Coverage Δ
datalad/core/distributed/clone.py 91.70% <100.00%> (-0.17%) ⬇️
datalad/core/distributed/tests/test_clone.py 97.33% <100.00%> (+0.10%) ⬆️
datalad/plugin/wtf.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/no_annex.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/add_readme.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/check_dates.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/plugin/export_archive.py 0.00% <0.00%> (-100.00%) ⬇️
... and 171 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 2fd46ee...e2330fc. Read the comment docs.

Test, that a configured push-url for ORA remote that matches the URL we
clone from, does prevent `clone` from reconfiguring the ORA remote. Just
like a matching `url` would.
See dataladgh-5628 for usage pattern and implications.
Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thx MUCH...this pained me substantially over the past weeks.

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through gh-5628 and stepped through the changed code (as triggered by the new test). Looks good to me too.

@kyleam kyleam merged commit fd7229a into datalad:master May 19, 2021
@yarikoptic yarikoptic added the semver-minor Increment the minor version when merged label Jun 2, 2021
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

4 participants