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: github - cycle to the next credential when we fail to obtain organization #4400

Merged
merged 2 commits into from Apr 22, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 14, 2020

Without it we immediately might fail if there is a known token which
does not have necessary permissions. Typically this would lead to just
a failure like

github.GithubException.BadCredentialsException: 401 {'message': 'Bad credentials', 'documentation_url': 'https://developer.github.com/v3'

now we will just go to the next credential while also providing an
informative log message on either we succeeded or not. So the interaction
would look like

$> datalad create-sibling-github --github-organization datalad-collection-1 testds
[WARNING] Having authenticated using token, we failed (401 {'message': 'Bad credentials', 'documentation_url': 'https://developer.github.com/v3'}) to access information about organization datalad-collection-1. We will try next authentication method (if any left available)
[INFO   ] Successfully obtained information about organization datalad-collection-1 using UserPassword(name='github', url='https://github.com/login') credential
.: github(-) [https://github.com/datalad-collection-1/testds.git (git)]
'https://github.com/datalad-collection-1/testds.git' configured as sibling 'github' for <Dataset path=/tmp/testds>

Closes #3946

Note: Merge to master would cause conflicts since it RFed many (but not all) relative imports to absolute, and thus merge of test_*py would require resolving them.

…nization

Without it we immediately might fail if there is a known token which
does not have necessary permissions.  Typically this would lead to just
a failure like

    github.GithubException.BadCredentialsException: 401 {'message': 'Bad credentials', 'documentation_url': 'https://developer.github.com/v3'

now we will just go to the next credential while also providing an
informative log message on either we succeeded or not.  So the interaction
would look like

	$> datalad create-sibling-github --github-organization datalad-collection-1 testds
	[WARNING] Having authenticated using token, we failed (401 {'message': 'Bad credentials', 'documentation_url': 'https://developer.github.com/v3'}) to access information about organization datalad-collection-1. We will try next authentication method (if any left available)
	[INFO   ] Successfully obtained information about organization datalad-collection-1 using UserPassword(name='github', url='https://github.com/login') credential
	.: github(-) [https://github.com/datalad-collection-1/testds.git (git)]
	'https://github.com/datalad-collection-1/testds.git' configured as sibling 'github' for <Dataset path=/tmp/testds>
"authentication method (if any left available)",
cred or "token", e, github_organization
)
continue
Copy link
Member Author

@yarikoptic yarikoptic Apr 14, 2020

Choose a reason for hiding this comment

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

this is the "heart" of the fix. The rest is just supportive changes ;)

Copy link
Collaborator

@kyleam kyleam left a comment

The description of the problem is clear and, with a cursory read, the change looks fine to me, though I'm not very familiar with this code. Left one inconsequential comment.



skip_if_no_github_cred = \
lambda f: skip_if(cond=not _get_github_cred().is_known)(f)
Copy link
Collaborator

@kyleam kyleam Apr 15, 2020

Choose a reason for hiding this comment

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

Is this lambda unnecessary?

Copy link
Member Author

@yarikoptic yarikoptic Apr 15, 2020

Choose a reason for hiding this comment

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

ha ha ha -- I was dancing around to make it work and have missed that it should be as simple as you suggested ;) Thank you for the review -- force pushing!

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #4400 into maint will decrease coverage by 0.04%.
The diff coverage is 87.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4400      +/-   ##
==========================================
- Coverage   89.63%   89.59%   -0.05%     
==========================================
  Files         275      275              
  Lines       37054    37076      +22     
==========================================
+ Hits        33215    33217       +2     
- Misses       3839     3859      +20     
Impacted Files Coverage Δ
datalad/support/github_.py 78.65% <81.81%> (-0.30%) ⬇️
datalad/consts.py 100.00% <100.00%> (ø)
datalad/support/tests/test_github_.py 87.14% <100.00%> (-12.86%) ⬇️
datalad/log.py 82.29% <0.00%> (-7.18%) ⬇️
datalad/tests/test_log.py 98.03% <0.00%> (-0.99%) ⬇️
datalad/metadata/search.py 88.66% <0.00%> (+0.18%) ⬆️
datalad/metadata/tests/test_search.py 100.00% <0.00%> (+4.82%) ⬆️

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 14faab4...5e87d5a. Read the comment docs.

@yarikoptic
Copy link
Member Author

yarikoptic commented Apr 22, 2020

comment was resolved, and the world will become a better place

@yarikoptic yarikoptic merged commit dd883be into datalad:maint Apr 22, 2020
8 of 11 checks passed
@yarikoptic yarikoptic added this to the 0.12.6 milestone Apr 23, 2020
@yarikoptic yarikoptic deleted the bf-github-org branch May 21, 2020
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

2 participants