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

ENH+BF(TST): create-sibling --group option #3098

Merged
merged 10 commits into from Jan 1, 2019
Merged

Conversation

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Dec 28, 2018

  1. ENH: create-sibling --group option

    When needing to create a group shared remote sibling, we need to be able
    to specify which group actually should it be (if directories setup does not
    enforce it).

  2. BF+RF: @skip_ decorators must not be used on generator tests

    Apparently some tests were simply not ran because decorated to be skipped,
    but our decorators do not work nicely with those generatorfunctions tests,
    so we must use them via their functional counterparts.

    Ideally those @skip_ helpers should be refactored since there is a repeating
    pattern and only the check is different. But I kept it "minimal" for now,
    and made them usable as functions within tests in such cases.

    There might now be test failures detected where they are still not used
    appropriately

yarikoptic added 3 commits Dec 28, 2018
When needing to create a group shared remote sibling, we need to be able
to specify which group actually should it be (if directories setup does not
enforce it).
Apparently some tests were simply not ran because decorated to be skipped,
but our decorators do not work nicely with those generatorfunctions tests,
so we must use them via their functional counterparts.

Ideally those @skip_ helpers should be refactored since there is a repeating
pattern and only the check is different.  But I kept it "minimal" for now,
and made them usable as functions within tests in such cases.

There might now be test failures detected where they are still not used
appropriately
@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Dec 29, 2018

this test (was not run for awhile) fails from travis but not locally

======================================================================
ERROR: datalad.downloaders.tests.test_http.test_download_ftp('ftp://ftp.gnu.org/README', None, 'This is ftp.gnu.org')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/virtualenv/python2.7.14/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/datalad/datalad/datalad/tests/utils.py", line 615, in newfunc
    return t(*(arg + (filename,)), **kw)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/tests/test_http.py", line 251, in check_download_external_url
    downloaded_path_ = downloader.download(url, path=d, size=s, overwrite=True)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/base.py", line 468, in download
    return self.access(self._download, url, path=path, **kwargs)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/base.py", line 145, in access
    result = method(url, **kwargs)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/base.py", line 369, in _download
    downloader_session = self.get_downloader_session(url)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/http.py", line 508, in get_downloader_session
    check_response_status(response, session=self._session)
  File "/home/travis/build/datalad/datalad/datalad/downloaders/http.py", line 112, in check_response_status
    raise AccessFailedError(err_msg)
AccessFailedError: Access to ftp://ftp.gnu.org/README has failed: status code 503

anyone knows some other good ftp resource we could test with?

@codecov
Copy link

@codecov codecov bot commented Dec 29, 2018

Codecov Report

Merging #3098 into master will decrease coverage by 28.58%.
The diff coverage is 65.38%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3098       +/-   ##
===========================================
- Coverage   90.34%   61.76%   -28.59%     
===========================================
  Files         248      248               
  Lines       32599    32602        +3     
===========================================
- Hits        29450    20135     -9315     
- Misses       3149    12467     +9318
Impacted Files Coverage Δ
datalad/downloaders/tests/test_http.py 32% <0%> (-54.72%) ⬇️
datalad/distribution/create_sibling.py 19.2% <0%> (-58.8%) ⬇️
datalad/tests/test_cmd.py 97.32% <100%> (+11.84%) ⬆️
datalad/distribution/tests/test_create_sibling.py 41.81% <20%> (-46.57%) ⬇️
datalad/tests/utils.py 82.9% <93.75%> (-6.36%) ⬇️
datalad/cmdline/tests/test_formatters.py 14.28% <0%> (-85.72%) ⬇️
datalad/interface/tests/test_save.py 16.66% <0%> (-83.34%) ⬇️
datalad/ui/tests/test_base.py 16.66% <0%> (-83.34%) ⬇️
datalad/distribution/tests/test_dataset.py 18.07% <0%> (-81.93%) ⬇️
datalad/plugin/addurls.py 18.32% <0%> (-81.37%) ⬇️
... and 161 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 5219d1a...b5235fb. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Dec 29, 2018

Codecov Report

Merging #3098 into master will increase coverage by 0.46%.
The diff coverage is 93.54%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3098      +/-   ##
=========================================
+ Coverage   90.34%   90.8%   +0.46%     
=========================================
  Files         248     248              
  Lines       32599   32666      +67     
=========================================
+ Hits        29450   29662     +212     
+ Misses       3149    3004     -145
Impacted Files Coverage Δ
datalad/tests/test_cmd.py 97.86% <100%> (+12.37%) ⬆️
datalad/distribution/create_sibling.py 85.76% <100%> (+7.76%) ⬆️
datalad/distribution/tests/test_create_sibling.py 98.6% <100%> (+10.22%) ⬆️
datalad/downloaders/tests/test_http.py 91.08% <85.71%> (+4.37%) ⬆️
datalad/tests/utils.py 89.28% <93.75%> (+0.02%) ⬆️
datalad/cmdline/main.py 78.16% <0%> (-0.47%) ⬇️
datalad/utils.py 87.48% <0%> (-0.05%) ⬇️
datalad/metadata/tests/test_search.py 93.52% <0%> (+0.29%) ⬆️
datalad/downloaders/http.py 85.31% <0%> (+2.77%) ⬆️
datalad/distribution/publish.py 87.94% <0%> (+3.54%) ⬆️
... and 6 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 5219d1a...697daf3. Read the comment docs.

kyleam added 2 commits Dec 29, 2018
test_download_ftp lost its "network" tag when skip_if_no_network was
moved to the body of the function.  The previous commit intended to
add it back, but the copy-and-pasted line wasn't adjusted.
Copy link
Contributor

@kyleam kyleam left a comment

Looks good to me. I left a couple of minor comments and pushed a couple of tweaks.

# Either it existed before or a new directory for a repo, set its
# group to a desired one if was provided
ssh("chgrp -R {} {}".format(
group,
Copy link
Contributor

@kyleam kyleam Dec 29, 2018

While not advisable or something we're likely to encounter, it seems like group can contain characters that would need to be shell escaped (that was based on quickly googling, so I may be wrong here). Should we use sh_quote here too?

Copy link
Member Author

@yarikoptic yarikoptic Dec 31, 2018

I guess it shouldn't hurt, will add now

@@ -202,6 +203,12 @@ def _create_dataset_sibling(
shared = CreateSibling._get_ds_remote_shared_setting(
delayed_super, name, ssh)

if group:
# Either it existed before or a new directory for a repo, set its
# group to a desired one if was provided
Copy link
Contributor

@kyleam kyleam Dec 29, 2018

I'm having trouble parsing this.

Copy link
Member Author

@yarikoptic yarikoptic Dec 31, 2018

I just wanted to say that the same command (chgrp) will be used regardless either it (directory/repository) already existed or we just created a new directory for it

Copy link
Member Author

@yarikoptic yarikoptic Dec 31, 2018

will try to make it more english since anyways need to push

"This is ftp.gnu.org"
# Started to throw 504 when on travis
try:
yield check_download_external_url, \
Copy link
Contributor

@kyleam kyleam Dec 31, 2018

I'd guess that the tests are still failing because you're trying to catch the exception around the yield rather than the actual invocation. Given that nothing else is yielded in this test, can you call check_download_external_url directly instead?

@yarikoptic yarikoptic merged commit 86d99fb into datalad:master Jan 1, 2019
5 checks passed
@yarikoptic yarikoptic deleted the enh-group branch Jan 6, 2019
yarikoptic added a commit that referenced this issue Feb 8, 2019
 A variety of bugfixes and enhancements

 ### Major refactoring and deprecations

 - All extracted metadata is now placed under git-annex by default.
   Previously files smaller than 20 kb were stored in git. ([#3109])
 - TODO: get_runner #3104 and pending #3131

 ### Fixes

 - Improved handling of long commands:
   - The code that inspected `SC_ARG_MAX` didn't check that the
     reported value was a sensible, positive number. ([#3025])
   - More commands that invoke `git` and `git-annex` with file
     arguments learned to split up the command calls when it is likely
     that the command would fail due to exceeding the maximum supported
     length. ([#3138])
 - The `setup_yoda_dataset` procedure created a malformed
   .gitattributes line. ([#3057])
 - [download-url] unnecessarily tried to infer the dataset when
   `--no-save` was given. ([#3029])
 - [rerun] aborted too late and with a confusing message when a ref
   specified via `--onto` didn't exist. ([#3019])
 - [run]:
   - `run` didn't preserve the current directory prefix ("./") on
      inputs and outputs, which is problematic if the caller relies on
      this representation when formatting the command. ([#3037])
   - Fixed a number of unicode py2-compatibility issues. ([#3035]) ([#3046])
   - To proceed with a failed command, the user was confusingly
     instructed to use `save` instead of `add` even though `run` uses
     `add` underneath. ([#3080])
 - Fixed a case where the helper class for checking external modules
   incorrectly reported a module as unknown. ([#3051])
 - [add-archive-content] mishandled the archive path when the leading
   path contained a symlink. ([#3058])
 - Following denied access, the credential code failed to consider a
   scenario, leading to a type error rather than an appropriate error
   message. ([#3091])
 - Some tests failed when executed from a `git worktree` checkout of the
   source repository. ([#3129])
 - During metadata extraction, batched annex processes weren't properly
   terminated, leading to issues on Windows. ([#3137])
 - [add] incorrectly handled an "invalid repository" exception when
   trying to add a submodule. ([#3141])
 - Pass `GIT_SSH_VARIANT=ssh` to git processes to be able to specify
   alternative ports in SSH urls

 ### Enhancements and new features

 - [search] learned to suggest closely matching keys if there are no
   hits. ([#3089])
 - [create-sibling] gained a `--group` option so that the caller can
   specify the file system group for the repository. ([#3098])
 - Interface classes can now override the default renderer for
   summarizing results. ([#3061])
 - [run]:
   - `--input` and `--output` can now be shortened to `-i` and `-o`.
     ([#3066])
   - Placeholders such as "{inputs}" are now expanded in the command
     that is shown in the commit message subject. ([#3065])
   - `interface.run.run_command` gained an `extra_inputs` argument so
     that wrappers like [datalad-container] can specify additional inputs
     that aren't considered when formatting the command string. ([#3038])
   - "--" can now be used to separate options for `run` and those for
     the command in ambiguous cases. ([#3119])
 - The utilities `create_tree` and `ok_file_has_content` now support
   ".gz" files. ([#3049])
 - The Singularity container for 0.11.1 now uses [nd_freeze] to make
   its builds reproducible.
 - A [publications] page has been added to the documentation. ([#3099])
 - `GitRepo.set_gitattributes` now accepts a `mode` argument that
   controls whether the .gitattributes file is appended to (default) or
   overwritten. ([#3115])
 - `datalad --help` now avoids using `man` so that the list of
   subcommands is shown.  ([#3124])

* tag '0.11.2': (124 commits)
  Changelog entry for GIT_SSH_VARIANT change
  ENH: Declare our GIT_SSH_COMMAND as GIT_SSH_VARIANT=ssh
  BF: sshconnector: Don't use ssh's port flag as scp's
  RF: sshconnector: Simplify shlex quote import
  CHANGELOG(0.11.2): Fix some typos
  [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.2 entries
  CHANGELOG: Do first pass for 0.11.2
  CHANGELOG: Add missing link target for download-url
  Start cooking the 0.11.2 release
  RF: appveyor - move test_install tests to be ran the last
  RF: text_type instead of str
  ENH(TST): provide my timing for the slow test
  BF(TST): adjust the test for the fact that AnnexRepo.add does not blow on nonexisting files
  ENH(TST): two tests which test quick or thorough for add failing with too long list of files
  BF: get stderr if present, otherwise just use str(e)
  BF: append out/err only if not empty/None
  Centrlize handling running commands with long files list in _run_command_files_split
  RF: remove minor duplication of -- handling, place all files handling closer to the call
  RF: move unrelated to try/except handling outside
  ENH+DOC: Report actual process handle, not just PID
  ...
@yarikoptic yarikoptic added this to the Release 0.11.2 milestone Feb 10, 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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants