Skip to content

CloneOptions does not contains a definition of CredentialsProvider,,, #2075

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

Closed
sgagnier-hearst opened this issue Jan 22, 2024 · 23 comments
Closed

Comments

@sgagnier-hearst
Copy link

Reproduction steps

var cloneOptions = new CloneOptions();
cloneOptions.CredentialsProvider = (_url, _user, _cred) => new UsernamePasswordCredentials { Username = credentials.Username, Password = credentials.Password };
Repository.Clone(repository, repofolder, CloneOptions);

Expected behavior

Private repository to get cloned to local hard drive.

Actual behavior

Error message CS1061 'CloneOptions' does not contain a definition for 'CredentialsProvider' and no accessible extension method 'CredentialsProvider' accepting a first argument of type 'CloneOptions' could be found (are you missing a using directive or an assembly reference?)

Version of LibGit2Sharp (release number or SHA1)

0.29.0

Operating system(s) tested; .NET runtime tested

Windows 11, .Net Framework 4.8

@bording
Copy link
Member

bording commented Jan 22, 2024

As mentioned in the change log, there were some breaking changes to CloneOptions as part of adding proxy support.

CredentialsProvider can now be found on the FetchOptions property on CloneOptions.

@kazakevich-alexei
Copy link

As mentioned in the change log, there were some breaking changes to CloneOptions as part of adding proxy support.

CredentialsProvider can now be found on the FetchOptions property on CloneOptions.

from this explanation, it is not clear at all what changes should be made. If you look at FetchOptions now, you won't see anything similar to CredentialsProvider there. Documentation, for example git-clone , immediately became useless.. Great, everything is very clear.... (no)

@mburtka
Copy link

mburtka commented Jan 29, 2024

As mentioned in the change log, there were some breaking changes to CloneOptions as part of adding proxy support.

CredentialsProvider can now be found on the FetchOptions property on CloneOptions.

The breaking change makes sense to me, but what was the rationale behind making the FetchOptions property on CloneOptions read only? I have a specific way of newing up FetchOptions in my app and not being able to assign means I'd have to change that logic to operate on an existing instance.

Do you have any opposition to exposing that either via setter or ctor? I'd be happy to submit a PR.

@gwsutcliffe
Copy link

I too am utterly confused by this change. Previously, I was creating a CloneOptions and setting CredentialsProvider in there. I can create a FetchOptions instead and set the credentials, but can't find any way to pass this into CloneOptions.

Please can someone share a fix? I'm passing in the credentials because the call failed without them.

@ethomson
Copy link
Member

ethomson commented Feb 21, 2024

@gwsutcliffe Does this work?

Repository.Clone("https://github.com/libgit2/TestGitRepository", "/tmp/testgitrepository",
    new CloneOptions {
        FetchOptions = {
            CredentialsProvider = ...
        }
    });

@gwsutcliffe
Copy link

gwsutcliffe commented Feb 21, 2024

FetchOptions is read only (see the comment by mburtka above). This is the problem - your solution would be fine if FetchOptions could be set (and I wouldn't be posting here).

I've rolled back to v0.28 for now so as not to break my application's functionality.

@ethomson
Copy link
Member

Ah, yes, I see, it was changed in November. In that case:

var cloneOptions = new CloneOptions();
cloneOptions.FetchOptions.CredentialsProvider = ...

should work, no?

(It looks like @mburtka was complaining about not being able to new up a FetchOptions - but I don't have any insight into the API design here.)

@mburtka
Copy link

mburtka commented Feb 21, 2024

Ah, yes, I see, it was changed in November. In that case:

var cloneOptions = new CloneOptions();
cloneOptions.FetchOptions.CredentialsProvider = ...

should work, no?

That does work, but I had a factory method to new up my customized `FetchOptions' using credentialing defined at startup. To upgrade I have to update that to operate on an existing instance:

var cloneOptions = new CloneOptions();
_fetchOptionsManager.Populate(cloneOptions.FetchOptions);

Instead of just newing up the FetchOptions with the CloneOptions. While it does work, it's ugly and is counter to all the other get/set behavior of the API.

@ethomson
Copy link
Member

Instead of just newing up the FetchOptions with the CloneOptions. While it does work, it's ugly and is counter to all the other get/set behavior of the API.

Right, like I said, I don't have any insight into the rationale here. I don't know if @bording would be willing to review a PR here to change this back to have a setter, or if there's a philosophical or architectural reason to keep it readonly?

@bording
Copy link
Member

bording commented Feb 22, 2024

Right, like I said, I don't have any insight into the rationale here. I don't know if @bording would be willing to review a PR here to change this back to have a setter, or if there's a philosophical or architectural reason to keep it readonly?

This was an intentional change on my part, primarily around the fact that I wanted to guard against the FetchOptions property from ever being null. I don't like APIs that have you create a type (CloneOptions in this case) that require you to create a new instance of a object, but then also require you to create instances of other nested types to actually make the original object valid.

That was why when I moved FetchOptions into a property of CloneOptions instead of how it was previously combining the various properties together, I decided to ensure the creation of the CloneOptions would also create the FetchOptions for it. And since an instance has been created for you, you would then set the individual options from the property as @ethomson demonstrated in #2075 (comment).

@mburtka
Copy link

mburtka commented Mar 3, 2024

@bording would you be amenable to having a constructor with either a null guard using the Ensure class used elsewhere or a coalesce-assign?

I new up my FetchOptions using a factory injected at startup (with credentialing, prune settings, etc..) and would prefer to keep that logic in the factory. But if you don't want to go that route I understand.

@davidsr2r
Copy link

Just creating links for the other issues by people facing the same issue.
#2087
#2088

To set the credentials correctly, see: #2075 (comment)

@No1e
Copy link

No1e commented May 24, 2024

Why do we have breaking change here? Breaking changes shall be introduce only in major versions. Or did I learn it wrong?

@audunrunhn
Copy link

Why do we have breaking change here? Breaking changes shall be introduce only in major versions. Or did I learn it wrong?

In semver 0.28 to 0.29 is considered a major version change and allows for breaking changes.

@PulsarFX
Copy link

In semver 0.28 to 0.29 is considered a major version change and allows for breaking changes.

I think you have to re-read this part of semver: https://semver.org/

  1. MINOR version when you add functionality in a backward compatible manner

@audunrunhn
Copy link

In semver 0.28 to 0.29 is considered a major version change and allows for breaking changes.

I think you have to re-read this part of semver: https://semver.org/

  1. MINOR version when you add functionality in a backward compatible manner

Yes, absolutely once you hit v1, but the version of this package starts with 0 so breaking something in 0.29 is fine.

@bording
Copy link
Member

bording commented Nov 24, 2024

Yes, absolutely once you hit v1, but the version of this package starts with 0 so breaking something in 0.29 is fine.

Yes, this is exactly why LibGit2Sharp is still on 0.x releases. No guarantees on API compatibility until 1.0.0.

@bording
Copy link
Member

bording commented Nov 24, 2024

@gwsutcliffe Does this work?

Repository.Clone("https://github.com/libgit2/TestGitRepository", "/tmp/testgitrepository",
    new CloneOptions {
        FetchOptions = {
            CredentialsProvider = ...
        }
    });

Earlier in the thread, it was said this doesn't work, however that isn't correct. The above code works just fine. Even though the FetchOptions property is get-only, you can still set properties on the instance as part of object initialization.

The thing that you can't do is have a pre-created FetchOptions instance and assign it to the property:

//This doesn't work
var fetchOptions = new FetchOptions();
var cloneOptions = new CloneOptions { FetchOptions = fetchOptions };

Personally, this limitation doesn't really bother me, but since it sounds like it would be useful for some people, I've gone ahead and added a constructor: #2132

While working on that, I noticed some other places that are inconsistent and might still be allowing nulls... but I guess that's just the nature of this old and creaky codebase: Consistently inconsistent! 😢

@JaviyaShivam
Copy link

JaviyaShivam commented Dec 3, 2024

But I want to clone repo with git Token, then what I have to do?

var cloneOptions = new CloneOptions();
cloneOptions.FetchOptions.CredentialsProvider = (_url, _user, _cred) =>
new UsernamePasswordCredentials
{
Username = "git",
Password = request.Token
};

Repository.Clone(request.RepoUrl, request.LocalPath, cloneOptions);

When I am using this this gives me an error, "LibGit2Sharp.LibGit2SharpException
HResult=0x80131500
Message=unexpected http status code: 403
Source=LibGit2Sharp "

@bording
Copy link
Member

bording commented Dec 3, 2024

@JaviyaShivam If you're using a GitHub personal access token, you still need to use your actual username, not git.

@JaviyaShivam
Copy link

Thanks @bording Its working for GitHub but what about the other platforms? What if I want to make my application open to work with all the git using platforms. Such as azure, google cloud etc.

@bording
Copy link
Member

bording commented Dec 3, 2024

Thanks @bording Its working for GitHub but what about the other platforms? What if I want to make my application open to work with all the git using platforms. Such as azure, google cloud etc.

I'm not sure I understand the question. You would provide appropriate credentials for each of those platforms in the way that they require you to provide. Nothing about how you would do that has changed in LibGit2Sharp.

@bording
Copy link
Member

bording commented Dec 3, 2024

At this point, I'm going to close this issue because the core premise of the issue has been resolved pretty clearly by showing how to set a CredentialsProvider for CloneOptions.

@bording bording closed this as completed Dec 3, 2024
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

No branches or pull requests