fix(lookup): fix metadata extraction and error handling #107
Conversation
- Extract metadata directly from resource methods instead of YAML round-trip which returned yaml.Node structure instead of actual data - Ignore NotFound and PermissionDenied errors in lookup callback, allowing templates to use fallback logic when resources are unavailable - Change items from map to slice for proper range iteration in templates This fixes lookup functionality in maintenance mode and Talos 1.12+. Co-Authored-By: Claude <[email protected]> Signed-off-by: Andrei Kvapil <[email protected]>
Summary of ChangesHello @kvaps, 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 refines the resource lookup mechanism by streamlining metadata extraction, enhancing error resilience for unavailable resources, and optimizing the data structure for template processing. These improvements ensure more robust and predictable behavior, particularly in scenarios like maintenance mode and with newer Talos versions, by preventing issues caused by improper data handling and unhandled access errors. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
📝 WalkthroughWalkthroughThe engine refactors resource metadata extraction to use the resource.Metadata() method directly instead of YAML parsing, adds gRPC status code handling to ignore NotFound and PermissionDenied errors during lookups, and changes the items collection structure from a map to a slice for improved template iteration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 introduces several valuable improvements to the lookup functionality. The metadata extraction is now more direct and efficient, avoiding the previous YAML round-trip. Error handling for lookups is more robust by ignoring 'NotFound' and 'PermissionDenied' errors, which allows for better fallback logic in templates. Additionally, changing the result collection from a map to a slice ensures predictable iteration order in templates. I have one suggestion to improve the clarity of the new error handling logic.
| errCode := status.Code(callError) | ||
| errStr := callError.Error() | ||
| if errCode == codes.NotFound || errCode == codes.PermissionDenied || | ||
| strings.Contains(errStr, "code = NotFound") || strings.Contains(errStr, "code = PermissionDenied") { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This logic can be made clearer by explicitly handling gRPC status errors separately from the string-matching fallback. Using status.FromError provides a clean way to check if the error is a gRPC status error. This refactoring improves readability by making the two cases (native gRPC error vs. string-based error) distinct. The string matching is brittle and should ideally only be a fallback when a proper status code isn't available.
| errCode := status.Code(callError) | |
| errStr := callError.Error() | |
| if errCode == codes.NotFound || errCode == codes.PermissionDenied || | |
| strings.Contains(errStr, "code = NotFound") || strings.Contains(errStr, "code = PermissionDenied") { | |
| return nil | |
| } | |
| st, ok := status.FromError(callError) | |
| if ok { | |
| code := st.Code() | |
| if code == codes.NotFound || code == codes.PermissionDenied { | |
| return nil | |
| } | |
| } else { | |
| // Fallback for non-gRPC errors that have gRPC error strings. | |
| errStr := callError.Error() | |
| if strings.Contains(errStr, "code = NotFound") || strings.Contains(errStr, "code = PermissionDenied") { | |
| return nil | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/engine/engine.go`:
- Around line 611-617: The code is doing brittle string matching against the
error text; remove that and rely solely on gRPC's status.Code for wrapped
errors: in the block referencing status.Code(callError) and strings.Contains,
drop the errStr := callError.Error() and both strings.Contains(...) checks and
keep only the comparison of errCode to codes.NotFound and codes.PermissionDenied
(returning nil in that case); also remove any now-unused import of "strings" if
present.
| // Ignore NotFound and PermissionDenied errors - resource doesn't exist or is not accessible | ||
| errCode := status.Code(callError) | ||
| errStr := callError.Error() | ||
| if errCode == codes.NotFound || errCode == codes.PermissionDenied || | ||
| strings.Contains(errStr, "code = NotFound") || strings.Contains(errStr, "code = PermissionDenied") { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
grpc-go v1.77.0 recommended way to detect NotFound PermissionDenied wrapped errors errors.As GRPCStatus
💡 Result:
In grpc-go v1.77.0, the simplest/recommended way to detect gRPC codes (including when the error is wrapped) is to use status.Code(err) and compare against codes.NotFound, codes.PermissionDenied, etc. status.Code (and status.FromError/status.Convert) will successfully extract the gRPC Status if the error is a gRPC status error or wraps one. [1]
import (
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
)
switch status.Code(err) {
case codes.NotFound:
// ...
case codes.PermissionDenied:
// ...
case codes.OK:
// err == nil
default:
// Unknown / other
}If you also want the full Status (message/details), use status.Convert(err) or status.FromError(err):
st := status.Convert(err) // convenience wrapper around FromError
if st.Code() == codes.NotFound { /* ... */ }Notes:
status.FromErrorexplicitly documents that it returns theStatuswhen the error “implementsGRPCStatus() *Status… or if err wraps a type satisfying this”. [1]- In v1.77.0, the module version is
1.77.0(grpc.Version). [2]
Sources: [1] [2]
🏁 Script executed:
# Inspect the file and context around the mentioned lines
head -n 650 pkg/engine/engine.go | tail -n 80Repository: cozystack/talm
Length of output: 2661
🏁 Script executed:
# Check imports in the file
head -n 50 pkg/engine/engine.goRepository: cozystack/talm
Length of output: 1470
🏁 Script executed:
# Search for other error handling patterns in the file
rg "status\.Code|strings\.Contains.*code" pkg/engine/engine.go -A 2 -B 2Repository: cozystack/talm
Length of output: 450
Remove brittle string matching; status.Code() is already the correct approach for wrapped errors.
The code already uses status.Code(callError), which is the recommended way in grpc-go v1.77.0 to detect gRPC codes on both direct and wrapped errors. The additional string matching with strings.Contains() is unnecessary and should be removed.
♻️ Simplify to
if callError != nil {
// Ignore NotFound and PermissionDenied errors - resource doesn't exist or is not accessible
errCode := status.Code(callError)
- errStr := callError.Error()
- if errCode == codes.NotFound || errCode == codes.PermissionDenied ||
- strings.Contains(errStr, "code = NotFound") || strings.Contains(errStr, "code = PermissionDenied") {
+ if errCode == codes.NotFound || errCode == codes.PermissionDenied {
return nil
}🤖 Prompt for AI Agents
In `@pkg/engine/engine.go` around lines 611 - 617, The code is doing brittle
string matching against the error text; remove that and rely solely on gRPC's
status.Code for wrapped errors: in the block referencing status.Code(callError)
and strings.Contains, drop the errStr := callError.Error() and both
strings.Contains(...) checks and keep only the comparison of errCode to
codes.NotFound and codes.PermissionDenied (returning nil in that case); also
remove any now-unused import of "strings" if present.
round-trip which returned yaml.Node structure instead of actual data
allowing templates to use fallback logic when resources are unavailable
This fixes lookup functionality in maintenance mode and Talos 1.12+.
Co-Authored-By: Claude [email protected]
Signed-off-by: Andrei Kvapil [email protected]
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.