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

BF: do not install post-update hook if no UI, allow to reconfigure full hierarchy, no submodule update --init in hook #3318

Merged
merged 5 commits into from Apr 12, 2019

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 11, 2019

  • Primary issue it resolves -- we have been installing post-update hook even when no web UI was asked to be published! So if your publish or create-sibling was ... a bit slow while just hanging there without returning any output -- you might know now why. Check if .git/hooks/post-update is there on remote and kill it (unless you do use web ui)
    • commit a4d40a6 first removes installation of the post-commit hook unless UI is set. It also avoids running post-update hook unless it is there on remote (and executable)
    • followup commit c680a40 fixes regression introduced by the previous one by enabling inheriting having hook or not from super.
    • the last one 22c5882 just for paranoid me to verify that hierarchy (not just 1 direct subdataset) is inheriting correctly
  • I guess it might run into some unpleasant conflicts due to all the rev_olutionization when we try to merge to master, so I guess expert revolutionist assistance would be required
  • I also later added 78e00dc which removes running submodule update --init from post-update hook. I do not think it is longer needed, and tests didn't fail for me locally supporting that. More in the commit msg
  • I also later added 01cf595 which allows to assure consistent hierarchy of subdatasets on the remote while inheriting settings from their supers. It would now not blow whenever working on the topmost dataset where there is no super
yarikoptic added 3 commits Apr 11, 2019
AFAIK post-update hook is useful only for updating web-ui information and running
"git update-server-info" for dummy web git backend.
It also calls "git submodule update --init" which might be generally not desirable.
Running that hook requires datalad on remote end, and could be quite time consuming
for any sizeable repo.  And there is no reason for that unless there is web-ui
somewhere in the super-dataset.

What this one breaks is creating missing remote siblings on a deployment WITH web ui,
while asking to inherit settings.  Next commit should work on that to bring it back.

Also we call out to all the hooks upon create-sibling at the end, but if there is no hook,
it should just skip that.
Previous commit disabled placing hook regardless either there is UI or not.
So before we always had a hook.  With this fix we would inherit from the super
…eated

I had a memory that it did not work correctly.  So ENHed existing test at the sacrifice
of a few seconds (but now marked it as @slow as it is regardless).
@yarikoptic yarikoptic changed the title BF: do not install post-update hook BF: do not install post-update hook if no web UI is asked to be installed as well Apr 11, 2019
All tests seems to pass, so I guess whatever issue was attempted to be solved
by this was resolved  by introducing correct traversal of the datasets
(create on going down, publish on going up), so no submodule update --init
call should be needed.

Side-effect it was causing is instantiating repositories on the remote
which might actually just be not a part of the dataset locally but just pointing
to remote urls (e.g. to github).
@codecov
Copy link

@codecov codecov bot commented Apr 11, 2019

Codecov Report

Merging #3318 into 0.11.x will decrease coverage by 34.93%.
The diff coverage is 6.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           0.11.x    #3318       +/-   ##
===========================================
- Coverage   90.77%   55.83%   -34.94%     
===========================================
  Files         252       94      -158     
  Lines       33036    14933    -18103     
===========================================
- Hits        29987     8338    -21649     
- Misses       3049     6595     +3546
Impacted Files Coverage Δ
datalad/distribution/create_sibling.py 26.64% <6.45%> (-58.46%) ⬇️
datalad/plugin/addurls.py 18.32% <0%> (-81.37%) ⬇️
datalad/support/globbedpaths.py 21.25% <0%> (-78.75%) ⬇️
datalad/interface/rerun.py 18.48% <0%> (-77.73%) ⬇️
datalad/interface/run.py 24.37% <0%> (-75.63%) ⬇️
datalad/plugin/export_archive.py 24.65% <0%> (-75.35%) ⬇️
datalad/support/due_utils.py 16.27% <0%> (-74.42%) ⬇️
datalad/distribution/publish.py 16.42% <0%> (-71.43%) ⬇️
datalad/interface/add_archive_content.py 19.23% <0%> (-71.16%) ⬇️
datalad/metadata/metadata.py 23.39% <0%> (-66.3%) ⬇️
... and 217 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 39e8e8b...78e00dc. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Apr 11, 2019

Codecov Report

Merging #3318 into 0.11.x will increase coverage by 0.06%.
The diff coverage is 94.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.11.x    #3318      +/-   ##
==========================================
+ Coverage   90.77%   90.83%   +0.06%     
==========================================
  Files         252      252              
  Lines       33036    33087      +51     
==========================================
+ Hits        29987    30055      +68     
+ Misses       3049     3032      -17
Impacted Files Coverage Δ
datalad/distribution/siblings.py 77.68% <100%> (+0.42%) ⬆️
datalad/distribution/tests/test_create_sibling.py 98.75% <100%> (+0.13%) ⬆️
datalad/distribution/create_sibling.py 85.89% <84.37%> (+0.79%) ⬆️
datalad/downloaders/tests/test_http.py 91.08% <0%> (+2.22%) ⬆️
datalad/downloaders/http.py 85.31% <0%> (+2.77%) ⬆️

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 39e8e8b...01cf595. Read the comment docs.

It is often desired to assure that we have consistent hierarchy
reflecting current one on the remote site.  But currently --inherit
would blow for the topmost super dataset since there is nothing to inherit
from.  With this change - we just issue a warning, and do not blow.
_DelayedSuper (not sure how useful it is really) will just report .super=None
in such cases so it could be verified that we can or cannot inherit
from it
@yarikoptic yarikoptic changed the title BF: do not install post-update hook if no web UI is asked to be installed as well BF: do not install post-update hook if no UI, allow to reconfigure full hierarchy, no submodule update --init in hook Apr 11, 2019
@mih
Copy link
Member

@mih mih commented Apr 12, 2019

LGTM

@yarikoptic yarikoptic merged commit ab64a6c into datalad:0.11.x Apr 12, 2019
5 checks passed
@yarikoptic yarikoptic deleted the bf-nopostupdate-if-no-ui branch Apr 12, 2019
kyleam added a commit that referenced this issue Apr 13, 2019
To resolve conflicts with changes from gh-3318, take changes from
0.11.x's side, but substitute in rev_create() and rev_save().
@yarikoptic yarikoptic added this to the Release 0.11.5 milestone Apr 15, 2019
yarikoptic added a commit to yarikoptic/datalad that referenced this issue Apr 15, 2019
In datalad#3318 (a4d40a6 in particular) I changed behavior
that no post-update hook  is installed by default.  And that is indeed desired since
unless it is a publicly facing via http git repository there is no need in such a hook.
BUT ATM the hook is unconfigurably melds two effects together:

  - making this repo accessible via dummy git http backend (by running git update-server-info)
  - and generating web listing metadata

There is no configuration to enable one but not another action in the hook.

So for this cast to still work with current functionality, we would need to enable web UI
which will establish the hook (and also install web UI index.html and all the needed JS
libraries).  In the long run I guess we should disentangle two aspects somehow and
grow the hook with needed feature depending on the configuration.
yarikoptic added a commit that referenced this issue May 28, 2019
0.11.5 (May 23, 2019) -- stability is not overrated

Should be faster and less buggy, with a few enhancements.

 Fixes

- [create-sibling][]  ([#3318][])
  - Siblings are no longer configured with a post-update hook unless a
    web interface is requested with `--ui`.
  - `git submodule update --init` is no longer called from the
    post-update hook.
  - If `--inherit` is given for a dataset without a superdataset, a
    warning is now given instead of raising an error.
- The internal command runner failed on Python 2 when its `env`
  argument had unicode values.  ([#3332][])
- The safeguard that prevents creating a dataset in a subdirectory
  that already contains tracked files for another repository failed on
  Git versions before 2.14.  For older Git versions, we now warn the
  caller that the safeguard is not active.  ([#3347][])
- A regression introduced in v0.11.1 prevented [save][] from committing
  changes under a subdirectory when the subdirectory was specified as
  a path argument.  ([#3106][])
- A workaround introduced in v0.11.1 made it possible for [save][] to
  do a partial commit with an annex file that has gone below the
  `annex.largefiles` threshold.  The logic of this workaround was
  faulty, leading to files being displayed as typechanged in the index
  following the commit.  ([#3365][])
- The resolve_path() helper confused paths that had a semicolon for
  SSH RIs.  ([#3425][])
- The detection of SSH RIs has been improved.  ([#3425][])

 Enhancements and new features

- The internal command runner was too aggressive in its decision to
  sleep.  ([#3322][])
- The "INFO" label in log messages now retains the default text color
  for the terminal rather than using white, which only worked well for
  terminals with dark backgrounds.  ([#3334][])
- A short flag `-R` is now available for the `--recursion-limit` flag,
  a flag shared by several subcommands.  ([#3340][])
- The authentication logic for [create-sibling-github][] has been
  revamped and now supports 2FA.  ([#3180][])
- New configuration option `datalad.ui.progressbar` can be used to
  configure the default backend for progress reporting ("none", for
  example, results in no progress bars being shown).  ([#3396][])
- A new progress backend, available by setting datalad.ui.progressbar
  to "log", replaces progress bars with a log message upon completion
  of an action.  ([#3396][])
- DataLad learned to consult the [NO_COLOR][] environment variable and
  the new `datalad.ui.color` configuration option when deciding to
  color output.  The default value, "auto", retains the current
  behavior of coloring output if attached to a TTY ([#3407][]).
- [clean][] now removes annex transfer directories, which is useful
  for cleaning up failed downloads. ([#3374][])
- [clone][] no longer refuses to clone into a local path that looks
  like a URL, making its behavior consistent with `git clone`.
  ([#3425][])
- [wtf][]
  - Learned to fall back to the `dist` package if `platform.dist`,
    which has been removed in the yet-to-be-release Python 3.8, does
    not exist.  ([#3439][])
  - Gained a `--section` option for limiting the output to specific
    sections and a `--decor` option, which currently knows how to
    format the output as GitHub's `<details>` section.  ([#3440][])

* tag '0.11.5': (96 commits)
  [DATALAD RUNCMD] make update-changelog
  Version boost and finalize CHANGELOG.md record
  ENH: new Makefile rule linkissues-changelog to link issues, which now will also be prerequisite for update-changelog
  CHANGELOG.md: Add entries for recently merged PRs
  ENH: require "distro" for python >= 3.8
  ENH: compat with python 3.8 which removed .dist -- try distro
  CLN: wtf: Remove unused (and duplicated) import
  DOC: wtf: Avoid double period in -S's description
  ENH: -D|--decor html_details -- to make it ready for pasting to github issue without clutter
  BF: assure bytes while giving to pyperclip upon its demand (on Py2)
  RF: move always present path + type "section" into "location" section, retain order of sections from cmdline
  RF: switch from nargs="*" to action=append for wtf -S
  ENH: wtf -S to specify which sections to query/display (by default -- all)
  MNT: Avoid invalid escape sequences in strings
  BF: export_to_figshare: Don't test identity of string literal
  BF(TST): do not assume user naiveness - treat any url-like looking path as a path
  BF: Check for /, \ or # in the username@hostname part while detecting SSHRI
  CHANGELOG.md: Add entry for gh-3374
  BF: revert back (remove) check for path being PathRI
  BF: list annex-transfer also in cmdline opt choice for "what"
  ...
yarikoptic added a commit that referenced this issue May 28, 2019
0.11.5 (May 23, 2019) -- stability is not overrated

Should be faster and less buggy, with a few enhancements.

 Fixes

- [create-sibling][]  ([#3318][])
  - Siblings are no longer configured with a post-update hook unless a
    web interface is requested with `--ui`.
  - `git submodule update --init` is no longer called from the
    post-update hook.
  - If `--inherit` is given for a dataset without a superdataset, a
    warning is now given instead of raising an error.
- The internal command runner failed on Python 2 when its `env`
  argument had unicode values.  ([#3332][])
- The safeguard that prevents creating a dataset in a subdirectory
  that already contains tracked files for another repository failed on
  Git versions before 2.14.  For older Git versions, we now warn the
  caller that the safeguard is not active.  ([#3347][])
- A regression introduced in v0.11.1 prevented [save][] from committing
  changes under a subdirectory when the subdirectory was specified as
  a path argument.  ([#3106][])
- A workaround introduced in v0.11.1 made it possible for [save][] to
  do a partial commit with an annex file that has gone below the
  `annex.largefiles` threshold.  The logic of this workaround was
  faulty, leading to files being displayed as typechanged in the index
  following the commit.  ([#3365][])
- The resolve_path() helper confused paths that had a semicolon for
  SSH RIs.  ([#3425][])
- The detection of SSH RIs has been improved.  ([#3425][])

 Enhancements and new features

- The internal command runner was too aggressive in its decision to
  sleep.  ([#3322][])
- The "INFO" label in log messages now retains the default text color
  for the terminal rather than using white, which only worked well for
  terminals with dark backgrounds.  ([#3334][])
- A short flag `-R` is now available for the `--recursion-limit` flag,
  a flag shared by several subcommands.  ([#3340][])
- The authentication logic for [create-sibling-github][] has been
  revamped and now supports 2FA.  ([#3180][])
- New configuration option `datalad.ui.progressbar` can be used to
  configure the default backend for progress reporting ("none", for
  example, results in no progress bars being shown).  ([#3396][])
- A new progress backend, available by setting datalad.ui.progressbar
  to "log", replaces progress bars with a log message upon completion
  of an action.  ([#3396][])
- DataLad learned to consult the [NO_COLOR][] environment variable and
  the new `datalad.ui.color` configuration option when deciding to
  color output.  The default value, "auto", retains the current
  behavior of coloring output if attached to a TTY ([#3407][]).
- [clean][] now removes annex transfer directories, which is useful
  for cleaning up failed downloads. ([#3374][])
- [clone][] no longer refuses to clone into a local path that looks
  like a URL, making its behavior consistent with `git clone`.
  ([#3425][])
- [wtf][]
  - Learned to fall back to the `dist` package if `platform.dist`,
    which has been removed in the yet-to-be-release Python 3.8, does
    not exist.  ([#3439][])
  - Gained a `--section` option for limiting the output to specific
    sections and a `--decor` option, which currently knows how to
    format the output as GitHub's `<details>` section.  ([#3440][])

* tag '0.11.5':
  BF: make test for url download more reliable in cases where connection fails
  RF: remove stale commented out duecredit in setup.py.  It has now its own section
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants