fix: refactor alicloud fc collector for better data structure #93
Conversation
Reviewer's GuideRefactored the Alicloud FC collector to improve the data structure by moving resource identifiers to row fields, emitting one record per function with its triggers, and implementing pagination for functions and triggers. Sequence diagram for paginated function and trigger collectionsequenceDiagram
participant Collector
participant FCClient
Collector->>FCClient: ListFunctionsRequest
FCClient-->>Collector: Functions + NextToken
loop while NextToken exists
Collector->>FCClient: ListFunctionsRequest (with NextToken)
FCClient-->>Collector: Functions + NextToken
end
loop for each Function
Collector->>FCClient: ListTriggersRequest(FunctionName)
FCClient-->>Collector: Triggers + NextToken
loop while NextToken exists
Collector->>FCClient: ListTriggersRequest(FunctionName, NextToken)
FCClient-->>Collector: Triggers + NextToken
end
end
Entity relationship diagram for FC function and triggerserDiagram
FUNCTION ||--o{ TRIGGER : has
FUNCTION {
string functionId
string functionName
}
TRIGGER {
string triggerId
string triggerName
}
Class diagram for refactored FC collector data structuresclassDiagram
class Detail {
+Function *fc20230330.Function
+Triggers []*fc20230330.Trigger
}
fc20230330.Function <|-- Detail: contains
fc20230330.Trigger <|-- Detail: contains multiple
Flow diagram for emitting records per function with triggersflowchart TD
A["Start: Collect FC Resources"] --> B["Describe Functions (paginated)"]
B --> C["For each Function"]
C --> D["Describe Triggers (paginated)"]
D --> E["Emit Detail record: Function + Triggers"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @j3ttt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the Alicloud Function Compute (FC) collector to improve its data structure and collection methodology. The primary goal is to provide a more accurate and comprehensive representation of FC resources by tightly coupling triggers with their respective functions and ensuring all relevant data is retrieved through pagination. These changes streamline the data model and enhance the collector's ability to gather complete information about FC instances. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- describeTriggers returns resp.Body.Triggers instead of the accumulated triggers slice, so earlier pages get discarded—return the aggregated ‘triggers’ variable instead.
- Pagination in describeFunction only handles one extra page; wrap the NextToken logic in a loop to ensure you collect all pages.
- In GetInstanceDetail, a single describeTriggers error will stop processing all functions; consider handling per-function errors without aborting the loop.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- describeTriggers returns resp.Body.Triggers instead of the accumulated triggers slice, so earlier pages get discarded—return the aggregated ‘triggers’ variable instead.
- Pagination in describeFunction only handles one extra page; wrap the NextToken logic in a loop to ensure you collect all pages.
- In GetInstanceDetail, a single describeTriggers error will stop processing all functions; consider handling per-function errors without aborting the loop.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Alicloud FC collector to improve the data structure, fetching functions and their associated triggers individually. This is a good structural improvement. However, I've identified critical bugs in the new pagination logic for both describeTriggers and describeFunction. The current implementation will lead to incomplete data collection if there are more results than fit on a single page (or two, in one case). My review includes suggestions to implement correct, robust pagination loops to fix these issues.
| func describeTriggers(ctx context.Context, cli *fc20230330.Client, name *string) (triggers []*fc20230330.Trigger) { | ||
| request := &fc20230330.ListTriggersRequest{} | ||
| resp, err := cli.ListTriggers(name, request) | ||
| if err != nil { | ||
| log.CtxLogger(ctx).Warn("ListCustomDomainsWithOptions error", zap.Error(err)) | ||
| log.CtxLogger(ctx).Warn("ListTriggersWithOptions error", zap.Error(err)) | ||
| return nil | ||
| } | ||
| triggers = append(triggers, resp.Body.Triggers...) | ||
|
|
||
| for resp.Body.NextToken != nil { | ||
| request.NextToken = resp.Body.NextToken | ||
| resp, err = cli.ListTriggers(name, request) | ||
| if err != nil { | ||
| log.CtxLogger(ctx).Warn("ListTriggersWithOptions error", zap.Error(err)) | ||
| return nil | ||
| } | ||
| triggers = append(triggers, resp.Body.Triggers...) | ||
| } | ||
|
|
||
| return result.Body.CustomDomains | ||
| return resp.Body.Triggers | ||
| } |
There was a problem hiding this comment.
The pagination logic in this function has a critical bug and can be simplified.
- Bug: The final
return resp.Body.Triggerson line 108 will only return the triggers from the last fetched page, discarding all triggers from previous pages that were appended to thetriggersslice. This will result in incomplete data. - Refactoring: The code for fetching and appending triggers is duplicated before and inside the loop. This can be consolidated into a single loop structure for better readability and maintainability.
I've suggested a refactoring that fixes the bug and cleans up the code.
func describeTriggers(ctx context.Context, cli *fc20230330.Client, name *string) (triggers []*fc20230330.Trigger) {
request := &fc20230330.ListTriggersRequest{}
for {
resp, err := cli.ListTriggers(name, request)
if err != nil {
log.CtxLogger(ctx).Warn("ListTriggers error", zap.Error(err))
return nil
}
if resp.Body != nil {
triggers = append(triggers, resp.Body.Triggers...)
if resp.Body.NextToken == nil {
break
}
request.NextToken = resp.Body.NextToken
} else {
break
}
}
return triggers
}1 participant
Thank you for your contribution to CloudRec!
What About:
java)go)opa)Description:
refactor alicloud fc collector for better data structure
Summary by Sourcery
Refactor the Alicloud FC collector to simplify data structures by modeling resources as individual functions with triggers and full pagination support
Enhancements: