warren830 commented on code in PR #8785:
URL:
https://github.com/apache/incubator-devlake/pull/8785#discussion_r2975648326
##########
backend/plugins/q_dev/api/connection.go:
##########
@@ -108,12 +108,22 @@ func GetConnection(input *plugin.ApiResourceInput)
(*plugin.ApiResourceOutput, e
// validateConnection validates connection parameters including Identity Store
fields
func validateConnection(connection *models.QDevConnection) error {
- // Validate AWS credentials
- if connection.AccessKeyId == "" {
- return errors.Default.New("AccessKeyId is required")
- }
- if connection.SecretAccessKey == "" {
- return errors.Default.New("SecretAccessKey is required")
+ // Default to access_key auth type if not specified
+ if connection.AuthType == "" {
+ connection.AuthType = "access_key"
Review Comment:
**Suggestion (medium):** A validation function ideally shouldn't mutate its
input. Setting the default `AuthType` here means `validateConnection` has a
side effect that callers may not expect. Consider moving the defaulting logic
to a dedicated `normalizeConnection()` step (or into the caller) and keeping
`validateConnection` pure.
##########
backend/plugins/q_dev/models/connection.go:
##########
@@ -24,9 +24,11 @@ import (
// QDevConn holds the essential information to connect to AWS S3
type QDevConn struct {
- // AccessKeyId for AWS
+ // AuthType determines how to authenticate with AWS: "access_key" or
"iam_role"
+ AuthType string `mapstructure:"authType" json:"authType"`
Review Comment:
**Suggestion (low):** `"access_key"` and `"iam_role"` are repeated across
validation, tests, migration, and both client files. Consider defining
constants:
```go
const (
AuthTypeAccessKey = "access_key"
AuthTypeIAMRole = "iam_role"
)
```
This prevents typo-driven bugs and makes future additions easier to grep for.
##########
backend/plugins/q_dev/tasks/s3_client.go:
##########
@@ -29,10 +29,14 @@ import (
func NewQDevS3Client(taskCtx plugin.TaskContext, connection
*models.QDevConnection) (*QDevS3Client, errors.Error) {
// 创建AWS session
- sess, err := session.NewSession(&aws.Config{
- Region: aws.String(connection.Region),
- Credentials:
credentials.NewStaticCredentials(connection.AccessKeyId,
connection.SecretAccessKey, ""),
- })
+ cfg := &aws.Config{
+ Region: aws.String(connection.Region),
+ }
+ // Only set static credentials for access_key auth; IAM role uses the
default credential chain
+ if !connection.IsIAMRoleAuth() {
+ cfg.Credentials =
credentials.NewStaticCredentials(connection.AccessKeyId,
connection.SecretAccessKey, "")
Review Comment:
**Nit (low):** This block is nearly identical to `identity_client.go` lines
51-59. A small helper like `newAWSSession(conn *models.QDevConnection, region
string) (*session.Session, error)` would DRY this up. Not blocking — just a
suggestion for a follow-up cleanup.
--
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]
