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+ENH: fixes on the way to get --existing=reconfigure functioning as it should #774

Merged
merged 8 commits into from
Sep 9, 2016

Conversation

yarikoptic
Copy link
Member

it is never enough of tests, but new ones revealing bugs are nice ;-)

@codecov-io
Copy link

codecov-io commented Sep 9, 2016

Codecov Report

Merging #774 into master will decrease coverage by 1.97%.
The diff coverage is 83.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #774      +/-   ##
=========================================
- Coverage   89.07%   87.1%   -1.98%     
=========================================
  Files         214     216       +2     
  Lines       19200   19415     +215     
=========================================
- Hits        17102   16911     -191     
- Misses       2098    2504     +406
Impacted Files Coverage Δ
datalad/distribution/add_sibling.py 85.86% <100%> (-0.31%) ⬇️
datalad/utils.py 90.22% <100%> (+0.02%) ⬆️
datalad/support/sshconnector.py 94.5% <100%> (+0.06%) ⬆️
datalad/tests/utils.py 89.9% <71.79%> (-1.52%) ⬇️
...ribution/create_publication_target_sshwebserver.py 78.68% <80%> (+1.24%) ⬆️
datalad/tests/test_tests_utils.py 96.19% <85.71%> (-0.54%) ⬇️
datalad/distribution/tests/test_target_ssh.py 99.17% <96.42%> (-0.83%) ⬇️
datalad/crawler/nodes/s3.py 32.82% <0%> (-57.26%) ⬇️
datalad/tests/test_s3.py 43.47% <0%> (-56.53%) ⬇️
datalad/downloaders/s3.py 41.73% <0%> (-51.31%) ⬇️
... and 19 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 fa6e6af...500f938. Read the comment docs.

@yarikoptic
Copy link
Member Author

@debanjum please review, and test with --existing=reconfigure settings to see if it actually works ok before I run it "in production". Theoretically (since still has TODO failing tests etc) now if I rerun with reconfigure it should also call the hooks so should regenerate all the meta-data etc. So -- you can even look into enhancing that test we were working on there

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.1%) to 84.959% when pulling 08ca396 on yarikoptic:bf-reconfigure into fa6e6af on datalad:master.

… get some error log msgs so disable that test on those systems
@coveralls
Copy link

coveralls commented Sep 9, 2016

Coverage Status

Coverage decreased (-4.1%) to 84.949% when pulling a19846d on yarikoptic:bf-reconfigure into fa6e6af on datalad:master.

@mih
Copy link
Member

mih commented Sep 9, 2016

LGTM, feel free to merge. Wasn't clear to me whether @debanjum already approved.

@yarikoptic
Copy link
Member Author

Well need to fix-up the test a bit first

@@ -297,6 +302,11 @@ def __call__(sshurl, target=None, target_dir=None,
lgr.error("Failed to add json creation command to post update hook.\n"
"Error: %s" % exc_str(e))

if not only_reconfigure:
# Initialize annex repo on remote copy if current_dataset is an AnnexRepo
if isinstance(dataset.repo, AnnexRepo):
Copy link
Contributor

@debanjum debanjum Sep 9, 2016

Choose a reason for hiding this comment

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

this should be datasets[current_dataset].repo instead of dataset.repo.
Also no need to pass datasets[current_dataset] to create_postupdate_hook function now

Copy link
Member Author

Choose a reason for hiding this comment

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

On Fri, 09 Sep 2016, Debanjum wrote:

In datalad/distribution/create_publication_target_sshwebserver.py:

@@ -297,6 +302,11 @@ def call(sshurl, target=None, target_dir=None,
lgr.error("Failed to add json creation command to post update hook.\n"
"Error: %s" % exc_str(e))

  •        if not only_reconfigure:
    
  •            # Initialize annex repo on remote copy if current_dataset is an AnnexRepo
    
  •            if isinstance(dataset.repo, AnnexRepo):
    

this should be datasets[current_dataset].repo instead of dataset.repo

great catch! thanks!

@debanjum
Copy link
Contributor

debanjum commented Sep 9, 2016

Can't trigger post-update hook on falkor due to the updated permissions on the virtualenv at datalad.org

@yarikoptic
Copy link
Member Author

On Fri, 09 Sep 2016, Debanjum wrote:

Can't trigger post-update hook on falkor due to the updated permissions on
the virtualenv at datalad.org

ok -- now it is available for you as well. but again, for testing we
should just make it happen on smaug. I will create some Dockerfile for
that I guess some time.

@yarikoptic
Copy link
Member Author

I am confused with codecov report -- it looks like previous tests somehow did run s3 and some other downloading related tests and then now they didn't... the same situation reported in other PRs so I don't think it is anything to do with us -- might try to figure it out some time. meanwhile merging this one

@yarikoptic yarikoptic merged commit 3461ecd into datalad:master Sep 9, 2016
@yarikoptic yarikoptic deleted the bf-reconfigure branch September 17, 2016 15:19
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

5 participants