[feat]: [CDS-73572]: Support List Repo Live Search for all git providers #261

adivishy1 · 2023-07-12T11:26:09Z

Have added code for supporting text based repo filtration for the following git providers:

Bitbucket cloud
Bitbucket server
GitHub
Gitlab

Note: Azure does not provide support for text based search.
Have tested out string based search all the above providers. For the other drivers that implement the GitService interface, have called the ListRepo method which means that its not filtering based on any repo being passed.

CLAassistant · 2023-07-12T11:26:16Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

bradrydzewski · 2023-07-12T12:51:44Z

scm/client.go

+	RepoListOptions struct {
+		SearchTerm      string
+		User            string
+		PageListOptions ListOptions
+	}


If you want the RepoListOptions struct to include the ListOptions you can use an embedded field, which is similar to inheritance:

RepoListOptions struct { + ListOptions SearchTerm string User string }

You can then access the ListOptions fields as if they were part of the RepoListOptions struct, while still being able to access the ListOptions struct

var opts RepoListOptions fmt.Println(opts.Page) fmt.Println(opts.ListOptions) // access ListOptions struct fmt.Println(opts.ListOptions.Page)

Thanks for this tip, wasnt aware about it, have incorporated the suggested change

abhinav-harness · 2023-07-13T07:21:36Z

Any chance you can also make it work for harness scm ?

adivishy1 · 2023-07-13T11:47:09Z

Any chance you can also make it work for harness scm ?

At present we wanted to implement for GIt providers mentioned in the pr, we can take this enhancement for harness scm separately

bradrydzewski · 2023-07-13T15:15:06Z

I noticed some files were added that use camelcase, for example:

scm/driver/bitbucket/testdata/reposFilter.json

This repository uses snake case (e.g. repos_filter.json). To ensure consistency with the rest of the repository, can you please update any files that use camel case to snake case? Thanks!

adivishy1 · 2023-07-14T04:56:19Z

I noticed some files were added that use camelcase, for example:
scm/driver/bitbucket/testdata/reposFilter.json
This repository uses snake case (e.g. repos_filter.json). To ensure consistency with the rest of the repository, can you please update any files that use camel case to snake case? Thanks!

Done

adivishy1 · 2023-07-14T05:16:37Z

I noticed some files were added that use camelcase, for example:
scm/driver/bitbucket/testdata/reposFilter.json
This repository uses snake case (e.g. repos_filter.json). To ensure consistency with the rest of the repository, can you please update any files that use camel case to snake case? Thanks!

Have incorporated the same changes to my previous pr for supporting live search for listing branches, hence few more file changes are appearing as part of this pr

tphoney

Can you make sure you are consistent adding comments for the V2 functionality if that SCM provider doesnt support search filters.
This also raises the question that when 3rd party developers that use this V2 functionality will have no idea whether the filters are working for all SCM providers, as they will always get a list result. But it wont be filtered.
I am not 100% sure on whether returning
return nil, nil, scm.ErrNotSupported is a better solution. @bradrydzewski thoughts ?

tphoney · 2023-07-14T09:57:35Z

scm/driver/gitea/repo.go

@@ -46,6 +46,10 @@ func (s *repositoryService) List(ctx context.Context, opts scm.ListOptions) ([]*
 	return convertRepositoryList(out), res, err
 }

+func (s *repositoryService) ListV2(ctx context.Context, opts scm.RepoListOptions) ([]*scm.Repository, *scm.Response, error) {
+	return s.List(ctx, opts.ListOptions)


you should have a similar comment to what you have in gitee

tphoney · 2023-07-14T09:58:39Z

scm/driver/gitee/repo.go

@@ -45,6 +45,9 @@ func (s *RepositoryService) List(ctx context.Context, opts scm.ListOptions) ([]*
 	res, err := s.client.do(ctx, "GET", path, nil, &out)
 	return convertRepositoryList(out), res, err
 }
+func (s *RepositoryService) ListV2(ctx context.Context, opts scm.RepoListOptions) ([]*scm.Repository, *scm.Response, error) {
+	return s.List(ctx, opts.ListOptions)


add a comment explaining why you are calling the list function.

adivishy1 · 2023-07-17T06:01:12Z

Can you make sure you are consistent adding comments for the V2 functionality if that SCM provider doesnt support search filters. This also raises the question that when 3rd party developers that use this V2 functionality will have no idea whether the filters are working for all SCM providers, as they will always get a list result. But it wont be filtered. I am not 100% sure on whether returning return nil, nil, scm.ErrNotSupported is a better solution. @bradrydzewski thoughts ?

I have added comments as to why we are calling the List api from V2.

For the 3rd party developers, they should continue to use the List api itself. IMO they should be knowing if the git provider that they are using provides search functionality or not and based on that they can update to the ListV2 apis. We can update our Harness docs as by calling out which all providers are supporting search filters.

If we go by return nil, nil, scm.ErrNotSupported , our scm service code will become ugly in which we will have to write code that will branch based on git providers to either call List or ListV2 apis.

As per the current implementation in this pr, the code will be flexible, in the scm service we just call the ListV2 api, we just add search term if passed or not. For the git providers that dont support search term, we return the complete list which will be in line with the existing prod behaviour, this will give us similar response as calling the List api

mohitg0795 · 2023-07-17T10:42:13Z

scm/client.go

@@ -71,8 +71,21 @@ type (
 	// BranchListOptions specifies optional branch search term and pagination
 	// parameters.
 	BranchListOptions struct {
-		SearchTerm      string
-		PageListOptions ListOptions
+		ListOptions


Changing this struct would be a backward incompatible change.

agreed, have reverted this

raghavharness · 2023-07-19T07:11:05Z

scm/driver/gitlab/repo.go

@@ -94,6 +94,13 @@ func (s *repositoryService) List(ctx context.Context, opts scm.ListOptions) ([]*
 	return convertRepositoryList(out), res, err
 }

+func (s *repositoryService) ListV2(ctx context.Context, opts scm.RepoListOptions) ([]*scm.Repository, *scm.Response, error) {


Add a comment explaining this API

raghavharness · 2023-07-19T07:27:24Z

scm/driver/github/util.go

@@ -22,6 +23,33 @@ func encodeListOptions(opts scm.ListOptions) string {
 	return params.Encode()
 }

+func formRepoListQueryParams(opts scm.RepoListOptions) string {


Can we keep the name consistent to encodeRepoListOptions for each provider?

For github we are not encoding the query params like we are doing for the other providers, hence different name.
When we encode the query params and try the same out in the curls and through the code, it doesnt work, hence no encoding query params for github

Okay can you once check for gitlab ?

for gitlab, in the existing List api, we are calling, encodeMemberListOptions hence in the ListV2 api we are calling encodeMemberRepoListOptions keeping the naming inline with the existing names

raghavharness · 2023-07-19T07:33:36Z

scm/driver/github/repo.go

+	path := fmt.Sprintf("search/repositories?%s", formRepoListQueryParams(opts))
+	out := new(searchRepositoryList)
+	res, err := s.client.do(ctx, "GET", path, nil, &out)
+	return convertSearchRepositoryList(out), res, err


We can use the existing convertRepositoryList method here ? just pass out.Repositories

adivishy1 changed the title ~~[feat]: [CDS-73572]: Support live search for all git providers"~~ [feat]: [CDS-73572]: Support List Repo Live Search for all git providers" Jul 12, 2023

bradrydzewski reviewed Jul 12, 2023

View reviewed changes

adivishy1 changed the title ~~[feat]: [CDS-73572]: Support List Repo Live Search for all git providers"~~ [feat]: [CDS-73572]: Support List Repo Live Search for all git providers Jul 13, 2023

adivishy1 force-pushed the master branch from 257383e to b0a6a30 Compare July 13, 2023 07:09

adivishy1 requested a review from bradrydzewski July 13, 2023 07:09

adivishy1 force-pushed the master branch from a13817c to 4ae5c55 Compare July 14, 2023 05:35

adivishy1 marked this pull request as ready for review July 14, 2023 06:26

tphoney suggested changes Jul 14, 2023

View reviewed changes

adivishy1 force-pushed the master branch from e2dfb61 to f31482e Compare July 17, 2023 10:30

mohitg0795 reviewed Jul 17, 2023

View reviewed changes

raghavharness requested changes Jul 19, 2023

View reviewed changes

adivishy1 force-pushed the master branch from e325dd3 to 9487748 Compare July 19, 2023 07:54

[feat]: [CDS-73572]: Support List Repo Live Search for all git providers

70cccf8

adivishy1 force-pushed the master branch from e1eae73 to 70cccf8 Compare July 19, 2023 10:01

raghavharness approved these changes Jul 19, 2023

View reviewed changes

mohitg0795 approved these changes Jul 19, 2023

View reviewed changes

rajatharanganath merged commit beec4da into drone:master Jul 19, 2023

tphoney added the enhancement label Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat]: [CDS-73572]: Support List Repo Live Search for all git providers #261

[feat]: [CDS-73572]: Support List Repo Live Search for all git providers #261

adivishy1 commented Jul 12, 2023 •

edited

Loading

CLAassistant commented Jul 12, 2023

bradrydzewski Jul 12, 2023

adivishy1 Jul 13, 2023

abhinav-harness commented Jul 13, 2023

adivishy1 commented Jul 13, 2023

bradrydzewski commented Jul 13, 2023

adivishy1 commented Jul 14, 2023

adivishy1 commented Jul 14, 2023

tphoney left a comment

tphoney Jul 14, 2023

adivishy1 Jul 17, 2023

tphoney Jul 14, 2023

adivishy1 Jul 17, 2023

adivishy1 commented Jul 17, 2023

mohitg0795 Jul 17, 2023

adivishy1 Jul 17, 2023

raghavharness Jul 19, 2023

adivishy1 Jul 19, 2023

raghavharness Jul 19, 2023

adivishy1 Jul 19, 2023

raghavharness Jul 19, 2023

adivishy1 Jul 19, 2023

raghavharness Jul 19, 2023

adivishy1 Jul 19, 2023

[feat]: [CDS-73572]: Support List Repo Live Search for all git providers #261

[feat]: [CDS-73572]: Support List Repo Live Search for all git providers #261

Conversation

adivishy1 commented Jul 12, 2023 • edited Loading

CLAassistant commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav-harness commented Jul 13, 2023

adivishy1 commented Jul 13, 2023

bradrydzewski commented Jul 13, 2023

adivishy1 commented Jul 14, 2023

adivishy1 commented Jul 14, 2023

tphoney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adivishy1 commented Jul 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adivishy1 commented Jul 12, 2023 •

edited

Loading