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: sshconnector: Get identity file with datalad.cfg.get() #3168

Merged
merged 1 commit into from Feb 16, 2019

Conversation

@kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 16, 2019

This reverts and replaces the sshconnector.py changes from
93c86517b (ENH: ssh: Support configuring an identity file,
2019-02-07).

93c86517b updated assure_initialized() to get datalad.ssh.identityfile
from its local ConfigManager instance and store that value as an
attribute for later use.  The motivation for retrieving the value in
assure_initialized() was to avoid the expense of calling
datalad.cfg.reload(force=True) with each get_connection() call.  In
turn, the motivation for using reload(force=True) was to prevent
test_ssh_custom_identity_file's patched DATALAD_SSH_IDENTITYFILE from
leaking into other tests [*].

The problem with this approach is that Python callers must set the
identity file before the first assure_initialized() call---which could
happen in SSHManager.get_connection() or _much earlier_ because
GitRepo.__init__() also calls assure_initialized().  After the first
assure_initialized() call, callers don't have a straightforward way of
changing the setting.

Instead let's use a plain datalad.cfg.get() call, without reloading,
within get_connection().  This gives Python callers the ability to
update the setting before _any_ get_connection() call, but they are
now responsible for reloading datalad.cfg if needed.

[*]: https://github.com/datalad/datalad/pull/3149#issuecomment-462918783
Re: #3149
This reverts and replaces the sshconnector.py changes from
93c8651 (ENH: ssh: Support configuring an identity file,
2019-02-07).

93c8651 updated assure_initialized() to get datalad.ssh.identityfile
from its local ConfigManager instance and store that value as an
attribute for later use.  The motivation for retrieving the value in
assure_initialized() was to avoid the expense of calling
datalad.cfg.reload(force=True) with each get_connection() call.  In
turn, the motivation for using reload(force=True) was to prevent
test_ssh_custom_identity_file's patched DATALAD_SSH_IDENTITYFILE from
leaking into other tests [*].

The problem with this approach is that Python callers must set the
identity file before the first assure_initialized() call---which could
happen in SSHManager.get_connection() or _much earlier_ because
GitRepo.__init__() also calls assure_initialized().  After the first
assure_initialized() call, callers don't have a straightforward way of
changing the setting.

Instead let's use a plain datalad.cfg.get() call, without reloading,
within get_connection().  This gives Python callers the ability to
update the setting before _any_ get_connection() call, but they are
now responsible for reloading datalad.cfg if needed.

[*]: datalad#3149 (comment)
Re: datalad#3149
@kyleam kyleam mentioned this pull request Feb 16, 2019
3 tasks
@codecov
Copy link

@codecov codecov bot commented Feb 16, 2019

Codecov Report

Merging #3168 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3168      +/-   ##
==========================================
+ Coverage    90.8%   90.83%   +0.02%     
==========================================
  Files         249      249              
  Lines       32781    32784       +3     
==========================================
+ Hits        29766    29778      +12     
+ Misses       3015     3006       -9
Impacted Files Coverage Δ
datalad/support/tests/test_sshconnector.py 99.29% <100%> (+0.02%) ⬆️
datalad/support/sshconnector.py 85.02% <100%> (-0.08%) ⬇️
datalad/downloaders/tests/test_http.py 91.08% <0%> (+2.22%) ⬆️

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 cf3f899...e43db77. Read the comment docs.

@yarikoptic yarikoptic merged commit 4edcc3a into datalad:master Feb 16, 2019
6 checks passed
@kyleam kyleam deleted the ssh-identity-timing branch Feb 16, 2019
kyleam added a commit to kyleam/datalad that referenced this issue Feb 16, 2019
yarikoptic added a commit that referenced this issue Feb 19, 2019
* prepare-0.11.3:
  ENH: Finalized changelog entry for 0.11.3
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.3 entries
  CHANGELOG: Mention #3168
  [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.3 entries
  CHANGELOG: Add entries for 0.11.3
yarikoptic added a commit to yarikoptic/datalad that referenced this issue Mar 4, 2019
* 0.11.x: (108 commits)
  ENH: Removed duplicate (and leading! ;)) Matteo from the past (Dartmouth)
  BF: annexrepo: Create temporary index under .git/
  BF(workaround): blow when extracting .gz if no 7z available for now
  RF/ENH(TST): test that extracted .gz file retains original name
  BF: convert patool's run cmd list into a string
  ENH(TST): test extraction of a single file (.gz)
  ENH(TST): test running with shell, comment that cmd as list + shell=True is probably not what you want
  ENH(TST,BK): test ability to compress/extract .gz
  Revert "BF(TMP): declare check_datasets_datalad_org failing on windows"
  Show must go on
  Boost version to 0.11.3
  BF(TST): skip if windows OR root; catch any exception
  ENH: Finalized changelog entry for 0.11.3
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.3 entries
  CHANGELOG: Mention datalad#3168
  ENH: sshconnector: Get identity file with datalad.cfg.get()
  [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.3 entries
  CHANGELOG: Add entries for 0.11.3
  BF/RF(TST): skip test if actual sudo chown call fails
  BF(TMP): declare check_datasets_datalad_org failing on windows
  ...
@yarikoptic yarikoptic added this to the Release 0.11.3 milestone Mar 14, 2019
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3': (35 commits)
  ENH: Finalized changelog entry for 0.11.3
  [DATALAD RUNCMD] CHANGELOG: Re-linkify 0.11.3 entries
  CHANGELOG: Mention #3168
  ENH: sshconnector: Get identity file with datalad.cfg.get()
  [DATALAD RUNCMD] CHANGELOG: Linkify 0.11.3 entries
  CHANGELOG: Add entries for 0.11.3
  BF/RF(TST): skip test if actual sudo chown call fails
  BF(TMP): declare check_datasets_datalad_org failing on windows
  BF(TST): replace not relevant trailing .pull test with .repo_info
  BF(BK): for some reason an exception on repo_info invocation isn't raised while on travis
  TST: test_ro_operations  via sudo (when possible)
  BF: allow to disallow git-annex branch merges for repo_info and use that in ls
  BF(unicode): use assure_unicode while formatting an exception
  BF(PY3): workaround for python3.4 on debian returning obnoxious SC_ARG_MAX
  BF: revert change for repo_info about not merging remote git-annex
  BF: set annex.merge-annex-branches=false for invocations not requiring updated remote information
  ENH: ssh: Support configuring an identity file
  ENH: SSHConnection: Support custom identity files
  ENH: ssh: Add identity_file parameter to get_connection_hash()
  TST+BF: sshconnector: Fix stale socket path construction
  ...
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3':
  BF(TST): skip if windows OR root; catch any exception
yarikoptic added a commit that referenced this issue Apr 6, 2019
 ## 0.11.3 (Feb 19, 2019) -- read-me-gently

 Just a few of important fixes and minor enhancements.

 ### Fixes

 - The logic for setting the maximum command line length now works
   around Python 3.4 returning an unreasonably high value for
   `SC_ARG_MAX` on Debian systems. ([#3165])

 - DataLad commands that are conceptually "read-only", such as
   `datalad ls -L`, can fail when the caller lacks write permissions
   because git-annex tries merging remote git-annex branches to update
   information about availability. DataLad now disables
   `annex.merge-annex-branches` in some common "read-only" scenarios to
   avoid these failures. ([#3164])

 ### Enhancements and new features

 - Accessing an "unbound" dataset method now automatically imports the
   necessary module rather than requiring an explicit import from the
   Python caller. For example, calling `Dataset.add` no longer needs to
   be preceded by `from datalad.distribution.add import Add` or an
   import of `datalad.api`. ([#3156])

 - Configuring the new variable `datalad.ssh.identityfile` instructs
   DataLad to pass a value to the `-i` option of `ssh`. ([#3149])
   ([#3168])

* tag '0.11.3':
  Boost version to 0.11.3
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