[GitHub] [incubator-devlake] klesh commented on a diff in pull request #4359: Surpport Gitlab v11 surpport

["klesh (via GitHub)" <gi...@apache.org>]

klesh commented on code in PR #4359:
URL: https://github.com/apache/incubator-devlake/pull/4359#discussion_r1099844157


##########
backend/plugins/gitlab/api/blueprint.go:
##########
@@ -44,21 +43,17 @@ func MakePipelinePlan(subtaskMetas []plugin.SubTaskMeta, connectionId uint64, sc
 	if err != nil {
 		return nil, err
 	}
-	token := strings.Split(connection.Token, ",")[0]
+	connection.Token = strings.Split(connection.Token, ",")[0]

Review Comment:
   This is confusing, do we support multiple tokens for gitlab connection? 



##########
backend/plugins/gitlab/api/blueprint.go:
##########
@@ -44,21 +43,17 @@ func MakePipelinePlan(subtaskMetas []plugin.SubTaskMeta, connectionId uint64, sc
 	if err != nil {
 		return nil, err
 	}
-	token := strings.Split(connection.Token, ",")[0]
+	connection.Token = strings.Split(connection.Token, ",")[0]
 
-	apiClient, err := api.NewApiClient(
+	apiClient, err := tasks.NewApiClientFromConnectionWithTest(
 		context.TODO(),
-		connection.Endpoint,
-		map[string]string{
-			"Authorization": fmt.Sprintf("Bearer %s", token),
-		},
-		10*time.Second,
-		connection.Proxy,
 		basicRes,
+		&connection.GitlabConn,

Review Comment:
   pass connection pointer is enough



##########
backend/plugins/gitlab/tasks/api_client.go:
##########
@@ -18,23 +18,72 @@ limitations under the License.
 package tasks
 
 import (
+	gocontext "context"
 	"net/http"
 	"strconv"
 	"time"
 
+	"github.com/apache/incubator-devlake/core/context"
 	"github.com/apache/incubator-devlake/core/errors"
 	"github.com/apache/incubator-devlake/core/plugin"
 	"github.com/apache/incubator-devlake/helpers/pluginhelper/api"
 	"github.com/apache/incubator-devlake/plugins/gitlab/models"
 )
 
-func NewGitlabApiClient(taskCtx plugin.TaskContext, connection *models.GitlabConnection) (*api.ApiAsyncClient, errors.Error) {
-	// create synchronize api client so we can calculate api rate limit dynamically
-	apiClient, err := api.NewApiClientFromConnection(taskCtx.GetContext(), taskCtx, connection)
+// NewApiClientFromConnectionWithTest creates ApiClient based on given connection.
+// and then it will test and try to correct the connection
+// The connection must
+func NewApiClientFromConnectionWithTest(
+	ctx gocontext.Context,
+	br context.BasicRes,
+	connection *models.GitlabConn,
+) (*api.ApiClient, errors.Error) {
+	connection.IsPrivateToken = false

Review Comment:
   `version checking` could be moved `PrepareApiClient` function, check https://github.com/apache/incubator-devlake/blob/25c68b456d0f89038b9ab70cb336156e70fff982/backend/plugins/feishu/models/connection.go#L35 for example.
   As matter of fact, we don't need `SetupAuthentication` if the token would not change during collection



##########
backend/plugins/gitlab/models/connection.go:
##########
@@ -18,13 +18,29 @@ limitations under the License.
 package models
 
 import (
+	"fmt"
+	"net/http"
+
+	"github.com/apache/incubator-devlake/core/errors"
 	helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api"
 )
 
 // GitlabConn holds the essential information to connect to the Gitlab API
 type GitlabConn struct {
 	helper.RestConnection `mapstructure:",squash"`
 	helper.AccessToken    `mapstructure:",squash"`
+	IsPrivateToken        bool `gorm:"-"`
+	UserId                int  `gorm:"-"`
+}
+
+// SetupAuthentication sets up the HTTP Request Authentication
+func (gak GitlabConn) SetupAuthentication(req *http.Request) errors.Error {

Review Comment:
   Consider using `PrepareApiClient` instead of `SetupAuthentication`, check https://github.com/apache/incubator-devlake/blob/25c68b456d0f89038b9ab70cb336156e70fff982/backend/plugins/feishu/models/connection.go#L35 for detail.



##########
backend/plugins/gitlab/tasks/account_collector.go:
##########
@@ -41,10 +43,23 @@ func CollectAccounts(taskCtx plugin.SubTaskContext) errors.Error {
 	logger := taskCtx.GetLogger()
 	logger.Info("collect gitlab users")
 
+	// test if we can use the "/projects/{{ .Params.ProjectId }}/members/all"
+	// before the gitlab v13.11 it can not be used
+	res, err := data.ApiClient.Get(fmt.Sprintf("/projects/%d/members/all", data.Options.ProjectId), url.Values{}, (http.Header)(nil))

Review Comment:
   This is not very descriptive, I can not see the intention of the code.
   I think we should detect the **version** of the Gitlab server, and then store it somewhere, like `task_data` maybe?
   And here, we should go different routes according to the **version**.
   By doing so, anybody could understand we are using a different API endpoint due to API versions.



##########
backend/plugins/gitlab/models/connection.go:
##########
@@ -18,13 +18,29 @@ limitations under the License.
 package models
 
 import (
+	"fmt"
+	"net/http"
+
+	"github.com/apache/incubator-devlake/core/errors"
 	helper "github.com/apache/incubator-devlake/helpers/pluginhelper/api"
 )
 
 // GitlabConn holds the essential information to connect to the Gitlab API
 type GitlabConn struct {
 	helper.RestConnection `mapstructure:",squash"`
 	helper.AccessToken    `mapstructure:",squash"`
+	IsPrivateToken        bool `gorm:"-"`
+	UserId                int  `gorm:"-"`
+}
+
+// SetupAuthentication sets up the HTTP Request Authentication
+func (gak GitlabConn) SetupAuthentication(req *http.Request) errors.Error {

Review Comment:
   Please use pointer to avoid memory copy



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@devlake.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org