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

create-sibling-{github,gogs,gitea,gin} as instances of GitHub-like platforms #5949

Merged
merged 20 commits into from
Sep 22, 2021

Conversation

mih
Copy link
Member

@mih mih commented Sep 2, 2021

This is a large PR that adds to new supported platforms (GIN, fixes #2680, and GOGS, fixes #5935), and reimplements the GitHub-platform support using the same code (fixes #5937).

Moreover, it comes with the following changes:

@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #5949 (326b2cd) into master (1e0f732) will decrease coverage by 32.88%.
The diff coverage is 34.65%.

❗ Current head 326b2cd differs from pull request most recent head 72ccd10. Consider uploading reports for the commit 72ccd10 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5949       +/-   ##
===========================================
- Coverage   90.32%   57.43%   -32.89%     
===========================================
  Files         308      312        +4     
  Lines       42111    42148       +37     
===========================================
- Hits        38035    24207    -13828     
- Misses       4076    17941    +13865     
Impacted Files Coverage Δ
datalad/cmdline/common_args.py 100.00% <ø> (ø)
datalad/cmdline/helpers.py 74.91% <ø> (+3.09%) ⬆️
...ad/distributed/tests/test_create_sibling_ghlike.py 0.00% <0.00%> (ø)
...talad/distributed/tests/test_create_sibling_gin.py 0.00% <0.00%> (ø)
...lad/distributed/tests/test_create_sibling_gitea.py 0.00% <0.00%> (ø)
...ad/distributed/tests/test_create_sibling_github.py 0.00% <0.00%> (ø)
...ad/distributed/tests/test_create_sibling_gitlab.py 0.00% <0.00%> (-100.00%) ⬇️
...alad/distributed/tests/test_create_sibling_gogs.py 0.00% <0.00%> (ø)
datalad/distribution/create_sibling_github.py 0.00% <0.00%> (-93.45%) ⬇️
datalad/interface/__init__.py 100.00% <ø> (ø)
... and 233 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 1e0f732...72ccd10. Read the comment docs.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Initial quick pass with minor comments. Didn't try yet and didn't comprehend the full scale of changes due to amount of code changes

datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distribution/create_sibling_github.py Outdated Show resolved Hide resolved
datalad/distribution/create_sibling_github.py Outdated Show resolved Hide resolved
@mih

This comment has been minimized.

@mih mih changed the title create-sibling-{github,gogs,gin} as instances of GitHub-like platforms create-sibling-{github,gogs,gitea,gin} as instances of GitHub-like platforms Sep 7, 2021
@mih mih added this to the 0.15.0 milestone Sep 7, 2021
@mih mih added enhancement UX user experience labels Sep 7, 2021
@mih mih removed this from the 0.15.0 milestone Sep 7, 2021
mih added a commit to mih/datalad that referenced this pull request Sep 8, 2021
Now there is --credential to identify a credential to be retrieved
from any credential provider (config, env, store), instead of passing a
token on the command line.

This addresses the gist of
datalad#5949 (comment)

This now also makes it possible to select a particular token out of a
set of multiple candidates for a specific site.
@mih

This comment has been minimized.

mih added a commit to mih/datalad that referenced this pull request Sep 8, 2021
As per discussion in datalad#5949

We better implement a `remove-sibling` equivalent, instead of custom
dances with interactive confirmation.

So maintain for github for now, but not introducing it for the new
platforms.
@mih

This comment has been minimized.

@mih mih marked this pull request as ready for review September 8, 2021 09:46
@yarikoptic
Copy link
Member

VCR tapes based integration tests were always a bit of a pain (since we didn't fully automate their sanitization), so not sure if worth the pains here. For anything on current list, but github, it could be real integration test against some docker container the actual service. But I guess could be considered out of scope of this PR. FWIW, relevant past issue ref #4079 which I will close now.

@mih mih added this to the 0.16.0 milestone Sep 14, 2021
Now there is --credential to identify a credential to be retrieved
from any credential provider (config, env, store), instead of passing a
token on the command line.

This addresses the gist of
datalad#5949 (comment)

This now also makes it possible to select a particular token out of a
set of multiple candidates for a specific site.
GIN uses SSH URL as primary clone URL, but our machinery prefers
an anonymous HTML URL for pull access.
_examples_ = [
dict(text="Use a new sibling on GIN as a common data source that is "
"auto-available when cloning from GitHub",
code_py="""\
Copy link
Member

Choose a reason for hiding this comment

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

I think, as a first basic example, a single command to just create a GitHub sibling would be helpful. This would also help to give the examples section a structure that makes it clear that the long, multi-command example is indeed a single example, not four. I just tried the command, and I was quite confused because with the lack of another example, it looks like there are four example commands, two of which use "create-sibling-gin".

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I pretty much made a protocol of some testing I did. All these commands should have more.

@adswa
Copy link
Member

adswa commented Sep 16, 2021

UX: When I don't have a token set up, the information that each command provides isn't really helpful.
For Github I get pointed to https://api.github.com, which is wrong and confusing. It should be https://github.com/settings/tokens, which can be hard to find in the web interface as it is reachable only via "Developer Settings".

❱ datalad create-sibling-github new-create-sibling-command-gh
You need to authenticate with None credentials. https://api.github.com provides information on how to gain access
token: 

For Gin I get pointed to https://gin.g-node.org, which is better, but it still takes me some time to find the correct page (https://gin.g-node.org/user/settings/applications)

❱ datalad create-sibling-gin new-create-sibling-command-gin -s gin
You need to authenticate with 'gin' credentials. https://gin.g-node.org provides information on how to gain access
token: 

@mih
Copy link
Member Author

mih commented Sep 17, 2021

For Github I get pointed to https://api.github.com, which is wrong and confusing. It should be https://github.com/settings/tokens, which can be hard to find in the web interface as it is reachable only via "Developer Settings".

Yes, that is suboptimal. However, the URL you gave is only correct for github.com, not for any other github deployment. Moreover, it is not always possible to construct the desired info URL from the API url.

I think the main issue here is that the credential system requires a URL here, instead of just textual instructions.

All platform support "self-hosting", i.e. can exist in multiple
instances. This makes it necessary to use a location-specific
default credential name, rather than a simple platform label
like 'github'. Otherwise successful retrieval of a previously stored,
but actually non-applicable credential could happen as default
behavior.
Otherwise internal logic when to call enter_new() must be reimplemented
by a caller.
@mih
Copy link
Member Author

mih commented Sep 17, 2021

I will shortly push an update that addresses the instruction problem:

% datalad create-sibling-gitea --credential no test 
An access token is required for https://gitea.com. Visit https://gitea.com/user/settings/applications to create a token

% datalad create-sibling-gogs --api https://mygogs.me.com --credential no test 
An access token is required for https://mygogs.me.com. Visit https://mygogs.me.com/user/settings/applications to create a token

% datalad create-sibling-gin --credential no test 
An access token is required for https://gin.g-node.org. Visit https://gin.g-node.org/user/settings/applications to create a token

% datalad create-sibling-github --credential no test 
An access token is required for https://api.github.com. Visit https://github.com/settings/tokens to create a token.

% datalad create-sibling-github --api https://api.mygithub.de --credential no test 
An access token is required for https://api.mygithub.de. Log into the platform, and visit [Account->Settings->Developer Settings->Personal access tokens->Generate new token]to create a new token.

@mih
Copy link
Member Author

mih commented Sep 20, 2021

This is complete now, and I have to detach. Please feel most welcome to add more examples, and doc tweaks.

The extension CI failures are unrelated to this PR and appear also in the reports at https://github.com/datalad/datalad-extensions

@mih mih added the merge-if-ok OP considers this work done, and requests review/merge label Sep 20, 2021
@mih mih requested a review from bpoldrack September 22, 2021 06:05
@bpoldrack
Copy link
Member

bpoldrack commented Sep 22, 2021

Before a code review, collecting observations from trying:

  • --dry-run doesn't seem to apply to implicit actions. That's not necessarily wrong, but also not necessarily obvious. Concrete case: dry-run with create-sibling-gin asked for token, subsequent dry-run doesn't anymore. It was stored the first time. Again that may even be desirable, but for a moment I was surprised, since my mental concept of a dry-run suggests no changes on disc.

  • GIN doesn't appear to work with a repo under an organization for me, but that's described in internal chat and looks more like an issue with GIN rather than datalad.
    Update: Issue seems to be that I'm not owner of the org. However, that is hidden at debug level:

    [DEBUG  ] GIN responded with <Response [403]> {'message': 'given user is not owner of organization', 'url': 'https://github.com/gogs/docs-api'} 
    [ERROR  ] HTTPError(403 Client Error: Forbidden for url: https://gin.g-node.org/api/v1/org/datalad/repos) (HTTPError) 
    
  • Github worked out just fine

Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Generally, I like it. Left a bunch of comments, though. From my point of view can be addressed (if applicable) after a merge as well, but at least the error reporting part should become an issue in that case.

metavar='[<org-name>/]<repo-(base)name>',
doc="""repository name, optionally including an '<organization>/'
prefix if the repository shall not reside under a user's namespace.
When operating recursively, a suffix will be appended to this name
Copy link
Member

Choose a reason for hiding this comment

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

What is that suffix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will adjust

datalad/distributed/create_sibling_ghlike.py Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Show resolved Hide resolved
datalad/distributed/create_sibling_ghlike.py Show resolved Hide resolved
datalad/distributed/create_sibling_gin.py Show resolved Hide resolved
datalad/distributed/create_sibling_gin.py Outdated Show resolved Hide resolved
datalad/distributed/create_sibling_gitea.py Show resolved Hide resolved
@mih
Copy link
Member Author

mih commented Sep 22, 2021

Thx for the review @bpoldrack ! I will fix some of the things you reported. I consider the implementation of a dry-run mode for credentials management out-of-scope for this PR. If necessary, I can disable the dry-run, and with it disable all tests that so far require no network. I consider that a waste, though.

rather than falling back on an exception. This cover exceeding
the permissions of the provided token now, and includes an explanation
of the situation in the error result.
@mih
Copy link
Member Author

mih commented Sep 22, 2021

OK, let's do this. All CI failures are unrelated issues present in master already. Thanks for all the feedback!

@mih mih merged commit a6ad558 into datalad:master Sep 22, 2021
@mih mih deleted the nf-gogs branch September 22, 2021 17:47
@bpoldrack
Copy link
Member

I consider the implementation of a dry-run mode for credentials management out-of-scope for this PR.

Yes, perfectly fine with that. I'm also not sure we'd want a change in that regard. Just noticed that it can be seen as a little weird.

adswa added a commit to adswa/datalad that referenced this pull request Sep 27, 2021
PR datalad#5949 deprecated the parameter --dryrun in favor of --dry-run in create-sibling{github,gogs,gitea,gin}.
This change fixes datalad#6011 by adding this deprecation and parameter alternative to create-sibling-gitlab as
well, in order to harmonize the usage of this parameter across the family of create-sibling-* commands.
adswa added a commit to adswa/datalad that referenced this pull request Sep 28, 2021
PR datalad#5949 deprecated the parameter --dryrun in favor of --dry-run in create-sibling{github,gogs,gitea,gin}.
This change fixes datalad#6011 by adding this deprecation and parameter alternative to create-sibling-gitlab as
well, in order to harmonize the usage of this parameter across the family of create-sibling-* commands.

TST: Discontinue use of deprecated dryrun in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd-create-sibling-github enhancement merge-if-ok OP considers this work done, and requests review/merge semver-minor Increment the minor version when merged UX user experience
Projects
None yet
4 participants