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

Let push honor multiple publication dependencies declared via siblings #6869

Merged
merged 3 commits into from
Jul 26, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jul 22, 2022

The documentation for declaring publication dependencies allows for two different ways to declare multiple dependencies:

  1. Declaring a single config item with a list of dependencies (space-separated)
  2. Declaring a config item multiple times with a single dependency each.

Previously push() only supported (1), while siblings() produced (2).

This change lets push() handle both scenarios.

Fixes #6867

Thanks to @LoannPeurey for the report!

mih added 2 commits July 22, 2022 10:08
Multiple publication dependencies configured via `siblings()` are not
honored by `push()`
The documentation for declaring publication dependencies
allows for two different ways to declare multiple dependencies:

1. Declaring a single config item with a list of dependencies
   (space-separated)
2. Declaring a config item multiple times with a single dependency
   each.

Previously `push()` only supported (1), while `siblings()` produced (2).

This change lets `push()` handle both scenarios.

Fixes datalad#6867

Thanks to @LoannPeurey for the report!
@mih mih added the semver-patch Increment the patch version when merged label Jul 22, 2022
mih added a commit to datalad/datalad-next that referenced this pull request Jul 22, 2022
@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #6869 (ddf7c42) into maint (cefecae) will increase coverage by 0.89%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            maint    #6869      +/-   ##
==========================================
+ Coverage   90.25%   91.15%   +0.89%     
==========================================
  Files         354      354              
  Lines       46113    46135      +22     
==========================================
+ Hits        41621    42053     +432     
+ Misses       4492     4082     -410     
Impacted Files Coverage Δ
datalad/core/distributed/push.py 92.57% <100.00%> (+0.02%) ⬆️
datalad/core/distributed/tests/test_push.py 98.94% <100.00%> (+<0.01%) ⬆️
datalad/cmd.py 85.43% <0.00%> (-8.67%) ⬇️
datalad/log.py 82.86% <0.00%> (-5.58%) ⬇️
datalad/api.py 90.24% <0.00%> (-4.50%) ⬇️
datalad/customremotes/tests/test_datalad.py 95.34% <0.00%> (-2.28%) ⬇️
datalad/tests/test_log.py 98.61% <0.00%> (-0.70%) ⬇️
datalad/distribution/create_sibling.py 69.80% <0.00%> (-0.20%) ⬇️
datalad/cli/common_args.py 100.00% <0.00%> (ø)
datalad/tests/test_dochelpers.py 100.00% <0.00%> (ø)
... and 21 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 cefecae...ddf7c42. Read the comment docs.

# multiple dependencies could come from multiple declarations
# of such a config items, but each declaration could also
# contain a white-space separated list of remote names
# see https://github.com/datalad/datalad/issues/6867
Copy link
Member

Choose a reason for hiding this comment

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

where do we document/allow for a white-space separated list of names?

Rationale for asking: Indeed ATM git does not allow white-space in the name of the remote, and it seems we do not "disallowed" it explicitly since just allow for git to error out:

$> datalad siblings add -s "my name" --url https://example.com
CommandError: 'git -c diff.ignoreSubmodules=none remote add 'my name' https://example.com' failed with exitcode 128
fatal: 'my name' is not a valid remote name

so if in the future git for some reason decides to support that (unlikely, but not impossible IMHO) such splitting by space might become buggy. So I wondered to ask where we already document/allow for that, and may be we should add some explicit checking for the name to not contain spaces (since could lead to fail of this added logic)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I cannot find a user-facing documentation that made me conclude that we allow(ed) for this. Maybe it was a comment, but I cannot find that either.

Copy link
Member

Choose a reason for hiding this comment

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

then since we didn't have that documented, and looking at original code above and behavior of ensure_list:

$> python -c 'from datalad.utils import ensure_list; print(ensure_list("w1 w2"))'
['w1 w2']

that it doesn't split by space, I think we never had such a feature -- should we really add it along with this? may be just a matter of adding get_all=True and be done?

Copy link
Member

Choose a reason for hiding this comment

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

@bpoldrack shines the archaeological light on the situation. We used to have get return all values by default, that caused issues, so get_all=False was introduced and thus broke prior behavior here which was to have all of the siblings pushed to (I guess the test didn't test for multiple). So I will push here a commit which replaces all the above with simple addition of get_all=True.

@bpoldrack shines the archaeological light on the situation. We used to have
get return all values by default, that caused issues, so get_all=False was
introduced and thus broke prior behavior here which was to have all of the
siblings pushed to (I guess the test didn't test for multiple). So I will push
here a commit which replaces all the above with simple addition of
get_all=True.
@yarikoptic
Copy link
Member

2 travis fails known, one is something to note -- might need @slow

___________________ test_nested_pushclone_cycle_allplatforms ___________________
Test passed but took too long to run: Duration 30.47114537500005s > 30.0s
----------------------------- Captured stdout call -----------------------------

@yarikoptic
Copy link
Member

with all of the above in mind, let's proceed !

@yarikoptic yarikoptic merged commit 1fa6441 into datalad:maint Jul 26, 2022
@yarikoptic yarikoptic deleted the bf-6867 branch October 14, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants