Pl 26239

[bhavya181]

[bhavya181]

I have created a interface ListRepoRequestParams which will contain all the extra parameters as per git provider, each git provider will implement this interface and have different fields in it, and then listV2 method has been defined which takes this interafce as one of the parameter.

[bradrydzewski]

I have some reservations. The purpose of this repository is to abstract common functionality across all git providers (github, bitbucket, gitlab, etc). This adds github-only functionality to all providers. I also noticed the naming conventions and coding style do not match Go conventions, nor do they match conventions used throughout this repository.

Perhaps you can provide some more details and describe the underlying requirement, and the team can help suggest whether or not the functionality belongs in this repository or the scm-service repository (in harness/core). They can also advise on implementations details (input structures, naming conventions, etc).

[bhavya181]

that's correct, the naming convention and coding style may not match with Go style, as its just a pseudocode to propose our approach, and coming on adding github-only functionality, we just want to abstract class which is query param to the method, so that each provider can put the extra ields, if they will require any in future for this listing api. will take team's help for same. Thanks

[tphoney]

I agree with @bradrydzewski changing the interface for all git providers is a bad approach and it makes it very confusing for non-harness users.

My suggestion before https://github.com/drone/go-scm/compare/master...tphoney-patch-1?quick_pull=1 is still a better approach do you concur @bradrydzewski ?

[bradrydzewski]

@bhavya181 I understand this is pusedo code, however, I do not understand what this pull request is trying to achieve which makes it difficult to review. Going forward please include a pull request description so that reviewers understand your proposal.

@tphoney the approach you are suggesting is consistent with our Stash implementation for list pagination. Is the goal of this pull request to enable list pagination, similar to Stash? If so, yes, your proposal is definitely preferable. It is important to use consistent implementation patterns across implementations, where possible. https://github.com/drone/go-scm/blob/b8f5fce3560d48de9171aa4ed73834a0db2d27ce/scm/driver/bitbucket/repo.go#L98:L102

[bhavya181]

@bradrydzewski so the aim is that, for github app there is a different end point for listing repos, so we just want to pass a boolean to understand where is github app or not in case of github, now according to @tphoney we should just pass that github app endpoint in listOptions.url, but then scm svc (harness-core) should pass an endpoint which should not be the repsonsibility of scm svc, ideally scm svc should just say we want repo list for github app, then go-scm should call the right end point to get details.

[bradrydzewski]

@TP I see where you are going with this, and it is a clever workaround. The only thing that gives me pause is these are two different endpoints with different behavior, and I'm not sure we want to set the precedent of allowing URL overrides for all endpoints, outside of Bitbucket pagination, where it cannot be avoided.

I would probably propose the following options:

1. Push vendor-specific logic to the Harness scm-service to keep this library clean and focused
2. Come up with a design strategy for how we want to support vendor-specific functionality

---

If we want option 2, this could be done by adding an endpoint only to the github.RepositoryService. There is no need to add this endpoint to the other drivers, or to register this method with the scm.RepositoryService interface.

// ListInstallation returns the repository list for the installation.
func (s *RepositoryService) ListInstallation(ctx context.Context, opts scm.ListOptions) ([]*scm.Repository, *scm.Response, error) {
	path := fmt.Sprintf("installation/repositories?%s", encodeListOptions(opts))
	out := []*repository{}
	res, err := s.client.do(ctx, "GET", path, nil, &out)
	return convertRepositoryList(out), res, err
}

The caller can invoke GitHub-specific endpoint by casting to the correct type:

client.Repos.(*github.RepositoryService).ListInstallation(ctx, opts)

---

but then scm svc (harness-core) should pass an endpoint which should not be the repsonsibility of scm svc, ideally scm svc should just say we want repo list for github app, then go-scm should call the right end point to get details.

@bhavya181 the purpose of this library is to expose common functionality across vendors through an abstraction layer. The RepositoryService.List endpoint returns a list of user repositories. This functionality is supported by all vendors, including github, gitlab, gogs, gitea, bitbucket, stash, etc.

However, returning a list of repositories by installation is vendor-specific behavior (github-only). It is not the responsibility of this library to expose or abstract vendor-specific functionality, it is the responsibility of the caller to make vendor-specific API calls. We can potentially expose some vendor-specific functions, however, it will be up to the caller (scm-service) to decide when to use vendor-specific functions; it will not be abstracted by this library.

[bhavya181]

@bradrydzewski this approach looks good, we can go ahead with this approach, what's say @tphoney?

Got your point regarding library, we should expose specific listing method only in github.repositoryService

[tphoney]

. The only thing that gives me pause is these are two different endpoints with different behavior The response is (as far as i have looked) the same for list repos and list repos for a github application. From the looks of it a github application only limits scope, for listing repositories.

If we do go with the approach of having a github application only method, that is good in terms of not polluting the go-scm service. 👍

however this will significantly increase complexity in the scm service. https://github.com/harness/harness-core/blob/develop/product/ci/scm/git/git.go#L537-L628 Now when you are being asked to list repo's, you cannot just call https://github.com/harness/harness-core/blob/develop/product/ci/scm/git/git.go#L549 as you will have to check if it is the github application. this will have to be done in multiple places. or you will have to add it as a separate function into the proto definition of the scm service.

[bradrydzewski]

however this will significantly increase complexity in the scm service

We need to add complexity somewhere (delegate, scm service, scm library). There are two options for how this could be implemented. First is that the scm-service exposes both List and ListInstances and the delegate chooses which to invoke. Second is the delegate passes in a boolean flag to the List endpoint, and there is an if statement to decides which function to invoke.

We are dealing with a fair amount of tech debt and complexity throughout the platform, but we have so far avoiding compromising this library, and I'd like to keep it that way.

If we do go with the approach of having a github application only method, that is good in terms of not polluting the go-scm service

the reason I am willing to consider github-specific methods is because we will probably soon need to support github checks, which is a github-only feature.

[bhavya181]

@bradrydzewski @tphoney, I think we all agree on adding a new method in go-scm and passing a boolean field from delegate to scm service, then scm service will make call based on boolean, should I go ahead with the implementation?