-
Notifications
You must be signed in to change notification settings - Fork 2
Feat/error #46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat/error #46
Conversation
- 在 errors 包中添加了 ErrID接口,用于获取错误 ID- 在 log 包中实现了 LoggerFromCtx 和 CreateLoggerCtx 函数,用于在上下文中传递日志对象 - 重构了 log包中的上下文键,提高了代码的可维护性 Signed-off-by: 百里(barry) <[email protected]>
- 在 Error 接口中添加 ID() string 方法 - 为 ErrCode、ErrMsg、ErrWrap 和自定义错误类型实现 ID 方法 - 在创建新错误时生成并分配唯一的错误 ID - 更新相关测试用例 Signed-off-by: 百里(barry) <[email protected]>
- 在errors包中新增GetErrorId函数,用于获取错误ID - 修改global和impl.log.go文件,增加错误ID的日志输出 - 优化了错误信息的格式化输出,包含错误ID和详细信息 Signed-off-by: 百里(barry) <[email protected]>
- 移除 global.go 文件中 errors 包的冗余导入 - 优化代码结构,提高代码可读性和维护性 Signed-off-by: 百里(barry) <[email protected]>
- 调整 err_simple.go 文件中的导入顺序,提高代码可读性 - 删除 marshaler.go 文件,简化错误处理逻辑 Signed-off-by: 百里(barry) <[email protected]>
- 新增 NewErr函数用于创建错误实例 - 实现了错误信息的自动 ID生成 - 增加了对空错误信息的处理 - 引入了 generic 和 xid 包 Signed-off-by: barry <[email protected]>
- 在 Result 类型中添加 WrapErr 方法,用于包装错误并添加标签 - 这些方法使得错误处理更加灵活和强大,便于在错误中添加上下文信息 Signed-off-by: barry <[email protected]>
- 新增 WithErr 方法,用于包装错误并添加调用者信息 -优化 proxy.go 文件的导入信息 Signed-off-by: barry <[email protected]>
- Remove import of aherrcheck package - Replace aherrcheck with resultchecker in error checking logic - Update import statement to use resultchecker package Signed-off-by: barry <[email protected]>
Signed-off-by: barry <[email protected]>
Signed-off-by: barry <[email protected]>
Signed-off-by: barry <[email protected]>
Signed-off-by: barry <[email protected]>
Signed-off-by: barry <[email protected]>
- Add PatchEnvs field to Resources struct for specifying env config files- Implement env config loading and initialization in LoadFromPath function - Create new EnvConfigMap type and EnvConf struct for handling env configs - Update runtime initialization to use new env config functionality- Add path_dir utility function for getting the working directory path Signed-off-by: barry <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @kooksee, 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 primarily focuses on internal infrastructure improvements, enhancing the robustness and maintainability of the codebase. Key changes include migrating to a new internal lifecycle management component, introducing a flexible mechanism for environment variable configuration through external files, and refining the error logging system for better consistency and detail. Additionally, the Go version and various project dependencies have been updated.
Highlights
- Centralized Lifecycle Management: I've introduced a new internal
component/lifecycle
package to manage application lifecycle hooks, replacing an external dependency (github.com/pubgo/lava/core/lifecycle
). This includes new helper functions likeWrapNoCtxErr
for cleaner integration. - Enhanced Configuration Loading: I've added support for loading environment variable definitions from external YAML files (
config/envs.go
). This allows dynamic patching of environment variables based on configuration, improving flexibility. - Improved Error Logging: I've refactored error logging mechanisms to centralize error detail formatting into a new
errDetail
utility function. This standardizes how errors are presented in logs, decoupling it from specific error types likeErrorProto
. - Go Version and Dependency Upgrades: I've updated the Go module to version 1.24 and performed a comprehensive upgrade of numerous third-party dependencies, ensuring the project uses more recent and potentially more stable library versions.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces multiple improvements and refactoring changes across the logging, environment, configuration, and utility modules. The main purpose is to enhance error handling, improve logging mechanisms, and standardize component lifecycle management.
Key changes include:
- Enhanced error handling and debugging by removing redundant debug stack prints and improving error detail formatting
- Environment variable processing improvements with prefix support and better validation
- Configuration system refactoring to support environment configuration maps and template syntax changes
- Component lifecycle management standardization across NATS and CloudEvent clients
Reviewed Changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
v2/result/util.go | Removes redundant debug.PrintStack() calls and refactors error checker usage |
log/impl.log.go | Refactors error handling to use new errDetail function and removes protobuf dependencies |
log/global.go | Updates error formatting logic to use new errDetail function |
env/init.go | Adds environment prefix support and enhanced logging during env loading |
config/config.go | Adds environment configuration map support and refactors template processing |
running/runtime.go | Adds device ID generation using machine ID library |
netutil/util.go | Improves local IP detection logic to focus on en0 interface |
component/lifecycle/lifecycle.go | Introduces new lifecycle management utilities |
Comments suppressed due to low confidence (1)
cmds/testmain/main.go:30
- [nitpick] The error message 'stdin is not a terminal' could be more descriptive about why this is problematic and what the user should do. Consider: 'this command requires an interactive terminal'.
return ctx, fmt.Errorf("stdin is not a terminal")
if netInterface.Name != "en0" { | ||
continue | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the network interface name 'en0' makes this code platform-specific to macOS. This will fail on Linux/Windows systems where interface names differ (e.g., 'eth0', 'wlan0'). Consider making this configurable or implementing cross-platform interface detection.
if netInterface.Name != "en0" { | |
continue | |
} |
Copilot uses AI. Check for mistakes.
if envKey == "" || | ||
strings.HasPrefix(envKey, "_") || | ||
strings.HasPrefix(envKey, "=") || | ||
!hasEnvPrefix(envKey, envPrefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When envPrefix is empty, hasEnvPrefix will always return false for non-empty keys, causing all environment variables to be ignored. The logic should allow all variables when no prefix is set.
!hasEnvPrefix(envKey, envPrefix) { | |
(envPrefix != "" && !hasEnvPrefix(envKey, envPrefix)) { |
Copilot uses AI. Check for mistakes.
@@ -177,7 +177,7 @@ func unmarshalOneOrList[T any](list *[]T, value *yaml.Node) error { | |||
if value.Kind == yaml.SequenceNode { | |||
return value.Decode(list) | |||
} | |||
return errors.Format("unmarshalled node: %v", value.Value) | |||
return errors.Errorf("unmarshalled node: %v", value.Value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'unmarshalled node' should be 'unmarshal node' - 'unmarshalled' is the past tense while 'unmarshal' is the action being attempted.
return errors.Errorf("unmarshalled node: %v", value.Value) | |
return errors.Errorf("unmarshal node: %v", value.Value) |
Copilot uses AI. Check for mistakes.
@@ -8,6 +8,6 @@ import ( | |||
|
|||
func SafeClose(closer io.Closer) { | |||
if err := closer.Close(); err != nil { | |||
log.Warn().Err(err).Msg("Close operation failed") | |||
log.Err(err).Msg("failed to safe close operation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message 'failed to safe close operation' is grammatically incorrect. It should be 'failed to safely close' or 'safe close operation failed'.
log.Err(err).Msg("failed to safe close operation") | |
log.Err(err).Msg("failed to safely close") |
Copilot uses AI. Check for mistakes.
log/util.go
Outdated
data, err1 := errData.MarshalJSON() | ||
if err1 != nil { | ||
return []byte(fmt.Sprintf("%s: %s", err1.Error(), pretty.Sprint(err))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable naming is confusing - 'err1' represents the marshaling error while 'err' is the original error parameter. Using 'err1.Error()' first and 'err' second in the format string makes the error message misleading about which error occurred where.
data, err1 := errData.MarshalJSON() | |
if err1 != nil { | |
return []byte(fmt.Sprintf("%s: %s", err1.Error(), pretty.Sprint(err))) | |
data, marshalErr := errData.MarshalJSON() | |
if marshalErr != nil { | |
return []byte(fmt.Sprintf("Original error: %s, Marshaling error: %s", pretty.Sprint(err), marshalErr.Error())) |
Copilot uses AI. Check for mistakes.
envData := env.Get(name) | ||
envData = strings.TrimSpace(lo.Ternary(envData != "", envData, cfg.Default)) | ||
if cfg.Required && envData == "" { | ||
panic("env " + cfg.Name + " is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panic message should use proper formatting instead of string concatenation. Consider using fmt.Sprintf for better readability: fmt.Sprintf("required environment variable %s is missing", cfg.Name).
panic("env " + cfg.Name + " is required") | |
panic(fmt.Sprintf("env %s is required", cfg.Name)) |
Copilot uses AI. Check for mistakes.
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a number of features and refactorings across the codebase. Key changes include adding a new testmain
CLI for testing, a version command, and a lifecycle
component for managing application lifecycle hooks. Configuration loading has been enhanced to support environment variable definitions from files (PatchEnvs
). Error handling and logging have been improved for more structured and detailed output.
I've identified a few critical issues that need to be addressed:
- The environment variable handling in
env/init.go
is unsafe as it clears all existing environment variables. - The
GetLocalIP
function innetutil/util.go
is not portable due to a hardcoded interface name. - The signal handling in
ctxutil/util.go
incorrectly attempts to catchSIGKILL
.
I've left detailed comments with suggestions for these issues. Addressing them will significantly improve the robustness and portability of the code.
func Signal() context.Context { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
ch := make(chan os.Signal, 1) | ||
signal.Notify(ch, syscall.SIGTERM, syscall.SIGINT, syscall.SIGQUIT, syscall.SIGKILL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signal.Notify
call includes syscall.SIGKILL
. However, SIGKILL
is a special signal that cannot be caught, handled, or ignored by a process. When a process receives SIGKILL
, it is terminated immediately without any chance for cleanup. Therefore, including it in this list is ineffective and misleading, as the signal handler goroutine will never receive it.
signal.Notify(ch, syscall.SIGTERM, syscall.SIGINT, syscall.SIGQUIT, syscall.SIGKILL) | |
signal.Notify(ch, syscall.SIGTERM, syscall.SIGINT, syscall.SIGQUIT) |
if !ok { | ||
_ = os.Unsetenv(rawKey) | ||
envKey := trim(kvs[0]) | ||
_ = os.Unsetenv(envKey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsetting every environment variable via os.Unsetenv(envKey)
is a very dangerous pattern. It can remove critical system-level environment variables (e.g., PATH
, HOME
, TMPDIR
) that the application or its dependencies might rely on, leading to unexpected behavior and hard-to-debug failures.
Instead of clearing the environment and selectively re-adding variables, it would be much safer to only modify the specific variables that need normalization, leaving the rest of the environment untouched.
if netInterface.Name != "en0" { | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding the interface name to en0
makes this function non-portable. This will only work on some systems (like macOS) and will fail on others (like Linux or Windows) where network interfaces have different names (e.g., eth0
, wlan0
).
A more robust approach is to iterate over all available network interfaces and filter based on their properties, such as checking if they are up and not a loopback interface.
if netInterface.Name != "en0" { | |
continue | |
} | |
// a more portable way to find a non-loopback interface | |
if netInterface.Flags&net.FlagLoopback != 0 || netInterface.Flags&net.FlagUp == 0 { | |
continue | |
} |
if strings.HasSuffix(help, "--help") || strings.HasSuffix(help, "-h") { | ||
return true | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of strings.HasSuffix
can lead to false positives. For example, an argument like --some-option-h
would be incorrectly identified as the -h
help flag. A direct string comparison is safer.
Additionally, this implementation is fragile because it only checks the last command-line argument. If a help flag is provided anywhere else in the command, it will not be detected.
if strings.HasSuffix(help, "--help") || strings.HasSuffix(help, "-h") { | |
return true | |
} | |
if help == "--help" || help == "-h" { | |
return true | |
} |
envData := env.Get(name) | ||
envData = strings.TrimSpace(lo.Ternary(envData != "", envData, cfg.Default)) | ||
if cfg.Required && envData == "" { | ||
panic("env " + cfg.Name + " is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The panic message uses cfg.Name
. However, the key of the envMap
is name
, which is the actual environment variable name being processed. If cfg.Name
is not set in the YAML configuration, the panic message will be unhelpful (e.g., "env is required"). It would be more robust to use the map key name
in the panic message.
panic("env " + cfg.Name + " is required") | |
panic("env " + name + " is required") |
No description provided.