Merged
Conversation
This reverts commit 74abede.
change docker-image.yaml, add lint.yaml
…/support-alibabacloud-resource
Feat/support alibabacloud resource: - ACS::DTS::Instance - ACS::ECI::ContainerGroup= - ACS::ECI::ImageCache
Feat/support alibabacloud resource
fix pagination support
Feat/support alibabacloud resource
Contributor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
Reviewer's GuideThe PR extends the Alibaba Cloud collector by adding support for ECS images and snapshots, API Gateway, SWAS instances, VPN connections, Bastionhost, DTS instances, and ECI container groups/image caches, integrating their clients, resource definitions, and detail-fetch logic across the service initializer and platform configuration. Sequence diagram for resource detail collection (e.g., ECS Image)sequenceDiagram
participant Collector
participant Services
participant ECSClient
participant AlibabaCloudAPI
Collector->>Services: InitServices(resourceType=ECSImage)
Services->>ECSClient: NewClientWithAccessKey(...)
Collector->>ECSClient: listImages()
ECSClient->>AlibabaCloudAPI: DescribeImages
AlibabaCloudAPI-->>ECSClient: Images list
ECSClient-->>Collector: Images
Collector->>Collector: Aggregate ImageDetail
Sequence diagram for API Gateway resource detail collectionsequenceDiagram
participant Collector
participant Services
participant CloudAPIClient
participant AlibabaCloudAPI
Collector->>Services: InitServices(resourceType=APIGateway)
Services->>CloudAPIClient: CreateCloudAPIClient(...)
Collector->>CloudAPIClient: listAPIs()
CloudAPIClient->>AlibabaCloudAPI: DescribeApis
AlibabaCloudAPI-->>CloudAPIClient: API list
loop for each API
Collector->>CloudAPIClient: describeAPIDetail(apiId)
CloudAPIClient->>AlibabaCloudAPI: DescribeApi
AlibabaCloudAPI-->>CloudAPIClient: Api detail
CloudAPIClient-->>Collector: APIGatewayDetail
end
Collector->>Collector: Aggregate APIGatewayDetail
Class diagram for ECS Image and Snapshot resource detailclassDiagram
class ImageDetail {
+ecs.Image Image
}
class SnapshotDetail {
+ecs.Snapshot Snapshot
}
ECSImage <|-- ImageDetail
ECSSnapshot <|-- SnapshotDetail
Class diagram for API Gateway, SWAS, Bastionhost, VPN Connection, DTS, and ECI resource detailsclassDiagram
class APIGatewayDetail {
+ApiSummary
+ApiInfo
}
class Detail {
+Instance
+FirewallRules
}
class BastionhostDetail {
+Instance
+InstanceAttribute
}
class VPNConnectionDetail {
+VpnConnection
}
class DTSInstanceDetail {
+MigrationJob
+MigrationJobDetail
}
class ECIContainerGroupDetail {
+ContainerGroup
}
class ECIImageCacheDetail {
+ImageCache
}
APIGateway <|-- APIGatewayDetail
SWAS <|-- Detail
Bastionhost <|-- BastionhostDetail
VpnConnection <|-- VPNConnectionDetail
DTSInstance <|-- DTSInstanceDetail
ECIContainerGroup <|-- ECIContainerGroupDetail
ECIImageCache <|-- ECIImageCacheDetail
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @j3ttt - I've reviewed your changes - here's some feedback:
- Consolidate the repeated pagination+worker patterns into shared helper functions to reduce duplication and centralize concurrency and error‐handling logic.
- Replace the growing switch in InitServices with a declarative resource–to–client registry to make it easier to add new resource types (e.g. Eflo) without forgetting to wire them up.
- Ensure detail fetchers honor context cancellation and close their task channels on error to avoid goroutine leaks or hanging workers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the repeated pagination+worker patterns into shared helper functions to reduce duplication and centralize concurrency and error‐handling logic.
- Replace the growing switch in InitServices with a declarative resource–to–client registry to make it easier to add new resource types (e.g. Eflo) without forgetting to wire them up.
- Ensure detail fetchers honor context cancellation and close their task channels on error to avoid goroutine leaks or hanging workers.
## Individual Comments
### Comment 1
<location> `collector/alicloud/collector/eci/container_group.go:55` </location>
<code_context>
+ req.PageNumber = tea.Int32(constant.DefaultPage)
+
+ var count int32 = 0
+ for {
+ resp, err := c.DescribeApis(req)
+ if err != nil {
</code_context>
<issue_to_address>
Potential risk of infinite loop if API returns a non-empty NextToken with no results.
Add a safeguard, such as a maximum iteration count or a check for empty results, to prevent infinite loops in this scenario.
</issue_to_address>
### Comment 2
<location> `collector/alicloud/collector/eci/image_cache.go:55` </location>
<code_context>
+ request := eci.CreateDescribeImageCachesRequest()
+ request.Scheme = "https"
+
+ response, err := cli.DescribeImageCaches(request)
+ if err != nil {
+ log.CtxLogger(ctx).Warn("DescribeImageCaches error", zap.Error(err))
+ return err
+ }
+
+ // 处理镜像缓存
+ for _, ic := range response.ImageCaches {
+ detail := ECIImageCacheDetail{
+ ImageCache: ic,
</code_context>
<issue_to_address>
No pagination handling for image cache listing.
Currently, only the first page of image caches is retrieved. Please add pagination handling to ensure all image caches are processed.
</issue_to_address>
### Comment 3
<location> `collector/alicloud/collector/dts/instance.go:79` </location>
<code_context>
+
+ count += int64(response.PageRecordCount)
+
+ if count >= response.TotalRecordCount || response.PageRecordCount == 0 {
+ break
+ }
</code_context>
<issue_to_address>
Potential for missing jobs if API returns inconsistent page counts.
To prevent missing jobs due to inconsistent API counts, also break the loop if the returned job list is empty.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
count += int64(response.PageRecordCount)
if count >= response.TotalRecordCount || response.PageRecordCount == 0 {
break
}
=======
count += int64(response.PageRecordCount)
if count >= response.TotalRecordCount || response.PageRecordCount == 0 || len(response.MigrationJobs.MigrationJob) == 0 {
break
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `collector/alicloud/collector/yundun/bastionhost.go:105` </location>
<code_context>
+ instances = append(instances, resp.Body.Instances...)
+ count += len(resp.Body.Instances)
+
+ if count >= int(*resp.Body.TotalCount) || len(resp.Body.Instances) < constant.DefaultPageSize {
+ break
+ }
</code_context>
<issue_to_address>
Potential nil pointer dereference for resp.Body.TotalCount.
Add a nil check for resp.Body.TotalCount before dereferencing to prevent a potential panic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+55
to
+64
| for { | ||
| response, err := cli.DescribeContainerGroups(request) | ||
| if err != nil { | ||
| log.CtxLogger(ctx).Warn("DescribeContainerGroups error", zap.Error(err)) | ||
| return err | ||
| } | ||
|
|
||
| // 处理容器组 | ||
| for _, cg := range response.ContainerGroups { | ||
| detail := ECIContainerGroupDetail{ |
There was a problem hiding this comment.
issue (bug_risk): Potential risk of infinite loop if API returns a non-empty NextToken with no results.
Add a safeguard, such as a maximum iteration count or a check for empty results, to prevent infinite loops in this scenario.
Comment on lines
+55
to
+62
| response, err := cli.DescribeImageCaches(request) | ||
| if err != nil { | ||
| log.CtxLogger(ctx).Warn("DescribeImageCaches error", zap.Error(err)) | ||
| return err | ||
| } | ||
|
|
||
| // 处理镜像缓存 | ||
| for _, ic := range response.ImageCaches { |
There was a problem hiding this comment.
issue: No pagination handling for image cache listing.
Currently, only the first page of image caches is retrieved. Please add pagination handling to ensure all image caches are processed.
Comment on lines
+77
to
+81
| count += int64(response.PageRecordCount) | ||
|
|
||
| if count >= response.TotalRecordCount || response.PageRecordCount == 0 { | ||
| break | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Potential for missing jobs if API returns inconsistent page counts.
To prevent missing jobs due to inconsistent API counts, also break the loop if the returned job list is empty.
Suggested change
| count += int64(response.PageRecordCount) | |
| if count >= response.TotalRecordCount || response.PageRecordCount == 0 { | |
| break | |
| } | |
| count += int64(response.PageRecordCount) | |
| if count >= response.TotalRecordCount || response.PageRecordCount == 0 || len(response.MigrationJobs.MigrationJob) == 0 { | |
| break | |
| } |
| instances = append(instances, resp.Body.Instances...) | ||
| count += len(resp.Body.Instances) | ||
|
|
||
| if count >= int(*resp.Body.TotalCount) || len(resp.Body.Instances) < constant.DefaultPageSize { |
There was a problem hiding this comment.
issue (bug_risk): Potential nil pointer dereference for resp.Body.TotalCount.
Add a nil check for resp.Body.TotalCount before dereferencing to prevent a potential panic.
Contributor
|
Important Installation incomplete: to start using Gemini Code Assist, please ask the organization owner(s) to visit the Gemini Code Assist Admin Console and sign the Terms of Services. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution to CloudRec!
What About:
java)go)opa)Description:
new resource type support: collection capabilities for ECS Images, ECS Snapshots, API Gateway instances, Simple Application Server (SWAS) instances, VPN Connections, and Bastionhost instances.
Summary by Sourcery
Add Go collector support for multiple new Alibaba Cloud resources by registering clients, constants and platform configuration, and implement detail functions for ECS Image, ECS Snapshot, API Gateway, SWAS, VPN Connection, Bastionhost, DTS, ECI ContainerGroup, and ECI ImageCache; refactor ACK cluster detail retrieval.
New Features:
Enhancements: