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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
