UX: push - provide specific error with details if push failed due to permission issue#7011
Merged
Merged
Conversation
yarikoptic
added
CHANGELOG-missing
github-actions
Bot
removed
the
CHANGELOG-missing
yarikoptic
added a commit
to yarikoptic/datalad
that referenced
this pull request
Sep 7, 2022
With that skip ci we make all CI, but appveyor skip, and then the status for that PR includes only appveyor and WiP requiring to explicitly look at prior commit status to assess full picture. I guess we should just rely on CIs to be able to cancel a possible existing run if more pushed to the PR branch Observed in datalad#7011 [skip ci] [appveyor skip]
yarikoptic
force-pushed
the
rf-push-perm-denied
branch
from
25c9679 to
88bf1b5
Compare
Member
|
A total of four different CI runs across two systems failed (plus the known container extension test run). I restarted them to get a sense of what is real here. |
mih
requested changes
Sep 8, 2022
mih
left a comment
mih
left a comment
Member
There was a problem hiding this comment.
It makes sense to catch such an error. But I have no idea how to trigger them.
| # property -- should not happen, but be robust | ||
| target.discard(None) | ||
| except CommandError as e: | ||
| if 'Please make sure you have the correct access rights' in e.stderr: |
Member
There was a problem hiding this comment.
It would be good for posterity to have at least some of these errors on record. In the commit message, here in the comment, doesn't matter. Personally, I have not seen such error, and based on the information given here, I have no idea what permissions where problematic. We know that these match-pieces-of-a-message approaches to error handling are fragile. Given that no test is added, we will never know whether this construct is still valid, and have to carry it forever, even when Git changes its behavior.
Member
Author
There was a problem hiding this comment.
damn right you are! not sure why I have not pasted it in its entirety. Now I have tried to reproduce by moving my ssh key aside, generating a new one and trying to push and got into another, very related but behaving entirely differently use case -- and apparently either we do not catch stdout there or it just goes directly to PTY somehow but we do not get it in stdout/stderr:
[DEBUG ] Opening MultiplexSSHConnection(ctrl_path=<<PosixPath('/ho++36 chars++9b')>>, sshri=<<SSHRI(hostname++26 chars++it')>>) by calling ['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/yoh/.cache/datalad/sock
ets/1f30269b', 'git@github.com']
Push: 0%| | 0.00/4.00 [00:00<?, ? Steps/s]
git@github.com: Permission denied (publickey).
> /home/yoh/proj/datalad/datalad-maint/datalad/core/distributed/push.py(457)_push()
-> if 'Please make sure you have the correct access rights' in e.stderr:
(Pdb) p e.stderr
None
*(Pdb) p e.stdout
None
but it does go to stderr:
$> ssh git@github.com
git@github.com: Permission denied (publickey).
$> ssh git@github.com 2>/dev/null
$>
so it started to question my whole fix! But I reproduced it and adjusted commit message with all the gory details: 9647b49
…permission issue
I kept getting an error like
$> datalad push
publish(impossible): . (dataset) [No push target given, and none could be auto-detected, please specify via --to]
action summary:
publish (impossible: 1)
and kept tuning the .git/config to specify extra options like push-remote etc,
but nothing helped. Only when looking at -l debug I saw that the issue has
nothing to do with default target not being given but rather with permissions
issue. IMHO "none coult be auto-detected" is although technically true that
datalad FAILS to auto-detect it, is semantically wrong since it could be
autodetected but datalad can't. That is why I decided that in cases whenever
it is a matter of permission issue, we can report that error in full.
With this change it looks like
$> datalad push
publish(impossible): . (dataset) [Attempt to push to default target resulted in following error. Address the error or specify different target with --to: ERROR: Permission to XXX/YYY.git denied to datalad-tester.
CommandError: 'ssh -o ControlPath=/home/yoh/.cache/datalad/sockets/1f30269b git@github.com 'git-receive-pack '"'"'XXX/YYY'"'"''' failed with exitcode 1
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.
]
action summary:
publish (impossible: 1)
To trigger such use case, make user for that repository on github to be known
among collaborators but have Read-Only permissions. So github would recognize
ssh key (so - not just some new key should be used) but user would not have
permission to push.
Interestingly, if the key is really unknown, somehow we are not getting stderr
to contain that error message
[DEBUG ] Opening MultiplexSSHConnection(ctrl_path=<<PosixPath('/ho++36 chars++9b')>>, sshri=<<SSHRI(hostname++26 chars++it')>>) by calling ['ssh', '-fN', '-o', 'ControlMaster=auto', '-o', 'ControlPersist=15m', '-o', 'ControlPath=/home/yoh/.cache/datalad/sock
ets/1f30269b', 'git@github.com']
Push: 0%| | 0.00/4.00 [00:00<?, ? Steps/s]
git@github.com: Permission denied (publickey).
> /home/yoh/proj/datalad/datalad-maint/datalad/core/distributed/push.py(457)_push()
-> if 'Please make sure you have the correct access rights' in e.stderr:
(Pdb) p e.stderr
None
*(Pdb) p e.stdout
None
although it is spit out to stderr:
$> ssh git@github.com
git@github.com: Permission denied (publickey).
$> ssh git@github.com 2>/dev/null
so this case is not handled, but at least this error message is passed through
to the user so they would see it before seeing a message that datalad could not
figure things out.
yarikoptic
force-pushed
the
rf-push-perm-denied
branch
from
88bf1b5 to
eb6c756
Compare
Codecov Report
@@ Coverage Diff @@
## maint #7011 +/- ##
==========================================
+ Coverage 75.41% 76.19% +0.78%
==========================================
Files 354 354
Lines 58751 58768 +17
Branches 6613 6614 +1
==========================================
+ Hits 44305 44778 +473
+ Misses 14431 13976 -455
+ Partials 15 14 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
christian-monch
pushed a commit
to christian-monch/datalad
that referenced
this pull request
Sep 16, 2022
With that skip ci we make all CI, but appveyor skip, and then the status for that PR includes only appveyor and WiP requiring to explicitly look at prior commit status to assess full picture. I guess we should just rely on CIs to be able to cancel a possible existing run if more pushed to the PR branch Observed in datalad#7011 [skip ci] [appveyor skip]
Member
|
Thanks for adding the additional info. |
Collaborator
|
PR released in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I kept getting an error like
and kept tuning the .git/config to specify extra options like push-remote etc,
but nothing helped. Only when looking at -l debug I saw that the issue has
nothing to do with default target not being given but rather with permissions
issue. IMHO "none could be auto-detected" is although technically true that
datalad FAILS to auto-detect it, is semantically wrong since it could be
autodetected but datalad can't. That is why I decided that in cases whenever
it is a matter of permission issue, we can report that error in full.
edit: details on how such errors were triggered were added to commit message. ATM it is 9647b49