Update Go version, resolve errors, and improve configuration#38
Update Go version, resolve errors, and improve configuration#38soulgarden merged 2 commits intomainfrom
Conversation
Updated Go version to 1.23.4 across workflows, Dockerfile, and `go.mod`. Enhanced error handling in consumers by switching to `RunE` and checking for context cancellation. Adjusted NATS queue defaults and removed unused configuration keys for cleaner setup.
WalkthroughThis pull request introduces a series of version upgrades and configuration adjustments across multiple files in the project. The changes primarily focus on updating the Go version to 1.23.4, modifying Docker base images to Alpine 3.21, updating workflow configurations, and refining error handling in command and service implementations. The modifications include updates to linting processes, NATS queue configurations, and dependency management, with an emphasis on improving robustness and maintaining consistency across the project's components. Changes
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
service/validator.go (1)
Line range hint
51-61: Potential inconsistency in error handling logicThe error handling for
IsValiddiffers from the unmarshal error handling - it doesn't return after sending the error response.if err != nil { zerolog.Ctx(ctx).Err(err).Msg("is valid") if err := msg.Respond([]byte(dictionary.TypeValidatedError)); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } + return err }cmd/validate_consumer.go (1)
Line range hint
47-51: Consider replacing os.Exit with error returnsEarly exits using
os.Exit(1)bypass deferred cleanup operations. Consider returning errors instead:natsConn, err := transport.NewConnection(cfg, serviceName) if err != nil { logger.Err(err).Msg("nats failed to establish connection") - os.Exit(1) + return fmt.Errorf("failed to establish NATS connection: %w", err) } defer natsConn.Close() mch := make(chan *nats.Msg, natsConn.Opts.SubChanLen) sub, err := natsConn.ChanQueueSubscribe(cfg.Nats.Queues.Validation, validatorWorkerName, mch) if err != nil { logger.Err(err).Msg("nats failed to subscribe") - os.Exit(1) + return fmt.Errorf("failed to subscribe to NATS queue: %w", err) }Also applies to: 57-61
service/generator.go (2)
Line range hint
52-60: Add missing return statement after HashPassword errorThe error handling for HashPassword is incomplete. After responding with an error, the code continues execution instead of returning:
hash, salt, err := s.passwordSvc.HashPassword(ctx, in.Password) if err != nil { zerolog.Ctx(ctx).Err(err).Msg("hash password") if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } + return err }
Line range hint
64-72: Add missing return statement after marshal errorSimilar to the HashPassword error handling, there's a missing return after marshal error:
body, err := out.MarshalMsg(nil) if err != nil { zerolog.Ctx(ctx).Err(err).Msg("marshal msg") if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } + return err }
🧹 Nitpick comments (8)
conf/conf.go (1)
Line range hint
1-71: Consider security and maintainability improvementsWhile not directly related to the current changes, here are some architectural suggestions for future improvements:
- Consider using environment variables or a secure secrets manager for sensitive information (DB credentials)
- The
New()function's error handling could be more graceful instead of callinglog.Fatal()- Consider adding validation for reasonable value ranges (e.g., for Password.Threads, DB.PoolSize)
Example approach for handling secrets:
type Master struct { - Password string `json:"password" required:"true"` + Password string `json:"password" required:"true" env:"DB_MASTER_PASSWORD"`Example approach for more graceful error handling:
-func New() *Config { +func New() (*Config, error) { c := &Config{} path := os.Getenv("CFG_PATH") if path == "" { path = "./conf/config.json" } if err := configor.New(&configor.Config{ErrorOnUnmatchedKeys: true}).Load(c, path); err != nil { - log.Fatal().Err(err).Msg("config validation errors") + return nil, fmt.Errorf("failed to load config: %w", err) } - return c + return c, nil }service/validator.go (1)
Line range hint
39-49: Consider consolidating error handling patternThe error handling pattern here could be simplified to avoid nested if statements and reduce code duplication.
if err != nil { zerolog.Ctx(ctx).Err(err).Msg("unmarshal msg") - - if err := msg.Respond([]byte(dictionary.TypeValidatedError)); err != nil { - zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") - - return err - } - - return err + return s.respondWithError(ctx, msg, dictionary.TypeValidatedError) }Consider adding a helper method:
func (s *Validator) respondWithError(ctx context.Context, msg *nats.Msg, errType string) error { if err := msg.Respond([]byte(errType)); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } return err }cmd/generate_consumer.go (2)
Line range hint
46-52: Consider graceful shutdown for NATS subscriptionWhile the subscription is properly deferred, consider implementing a more graceful shutdown pattern.
defer sub.Unsubscribe() +// Add drain to ensure pending messages are processed +defer func() { + if err := natsConn.Drain(); err != nil { + logger.Err(err).Msg("failed to drain connection") + } +}()
Line range hint
54-69: Consider structured logging for generator process errorsThe error logging could be enhanced with additional context.
-logger.Err(err).Msg("generator process") +logger.Err(err). + Str("worker", generatorWorkerName). + Str("queue", cfg.Nats.Queues.Generation). + Msg("generator process failed")cmd/validate_consumer.go (1)
Line range hint
38-39: Consider adding timeout for graceful shutdownThe command currently relies solely on signal handling for shutdown. Consider adding a timeout mechanism to ensure graceful shutdown completes within a reasonable time:
var ( cmdManager = service.NewManager(&logger) ctx, _ = cmdManager.ListenSignal() + shutdownCtx, cancel = context.WithTimeout(ctx, 30*time.Second) ) + defer cancel()service/generator.go (1)
Line range hint
23-89: Consider extracting message processing logicThe Process method is quite long and handles multiple responsibilities. Consider extracting the message processing logic into a separate method for better maintainability:
+ func (s *Generator) processMessage(ctx context.Context, msg *nats.Msg) error { + msgID := msg.Header.Get("id") + ctx = zerolog.Ctx(ctx).With(). + Str("id", msgID). + Logger(). + WithContext(context.WithValue(ctx, dictionary.ID, msgID)) + + // ... rest of the message processing logic ... + } func (s *Generator) Process(ctx context.Context, delivery <-chan *nats.Msg) error { for { select { case msg, ok := <-delivery: if !ok { return dictionary.ErrDeliveryChannelClosed } - // ... current message processing logic ... + if err := s.processMessage(ctx, msg); err != nil { + return err + } case <-ctx.Done(): return ctx.Err() } } }.github/workflows/main.yml (2)
25-25: Consider using matrix strategy for Go versionsTo ensure compatibility across Go versions, consider using a matrix strategy:
jobs: golangci: name: lint runs-on: ubuntu-latest + strategy: + matrix: + go-version: [1.23.4, 1.24.0] steps: - uses: actions/setup-go@v5 with: - go-version: 1.23.4 + go-version: ${{ matrix.go-version }}Also applies to: 60-60
96-96: Consider adding latest tag for Docker imagesFor better Docker image versioning, consider adding a latest tag:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.11 + tags: | + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.11 + ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-latestAlso applies to: 106-106
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
.github/workflows/main.yml(4 hunks)Dockerfile(2 hunks)Makefile(2 hunks)cmd/generate_consumer.go(3 hunks)cmd/validate_consumer.go(3 hunks)conf/conf.go(1 hunks)conf/config.github.json(0 hunks)conf/config.json(0 hunks)go.mod(2 hunks)main.go(1 hunks)service/generator.go(1 hunks)service/validator.go(1 hunks)transport/nats.go(1 hunks)
💤 Files with no reviewable changes (2)
- conf/config.github.json
- conf/config.json
✅ Files skipped from review due to trivial changes (4)
- main.go
- transport/nats.go
- go.mod
- Dockerfile
🔇 Additional comments (7)
conf/conf.go (1)
17-18: Verify queue name changes across the system
The queue names have been namespaced with "pass." prefix, which is a good practice for better organization. However, we should ensure this change is synchronized with all consumers and producers.
Let's verify the usage of these queue names across the codebase:
Consider documenting the queue naming convention in the project's documentation to maintain consistency for future queue additions.
✅ Verification successful
Let me gather more specific information about how these queue names are actually used in the code.
Let me check one final aspect - the actual NATS subscription code in the consumer files we discovered.
Queue name changes are properly synchronized across the system
The queue name changes are correctly implemented and synchronized across the codebase:
- The new queue names (
pass.validationandpass.generation) are defined inconf/conf.go - The consumer services correctly use these queue names through the config:
- Validator uses
cfg.Nats.Queues.Validationfor subscription - Generator uses
cfg.Nats.Queues.Generationfor subscription
- Validator uses
- The worker names in the consumers (
swup.pass.validationandswup.pass.generation) already follow the same namespacing convention - No hardcoded references to the old queue names were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any hardcoded references to the old queue names and verify the new queue names are used consistently
# Check for old queue names
echo "Checking for old queue names..."
rg -l "\"validation\"" --type go
rg -l "\"generation\"" --type go
# Check for new queue names
echo "Checking for new queue names..."
rg -l "\"pass.validation\"" --type go
rg -l "\"pass.generation\"" --type go
# Look for NATS subscribe/publish calls that might use these queues
echo "Checking NATS operations..."
ast-grep --pattern 'Subscribe($$$)'
ast-grep --pattern 'QueueSubscribe($$$)'
ast-grep --pattern 'Publish($$$)'
Length of output: 521
Script:
#!/bin/bash
# Let's look at the actual usage context of these queue names
echo "Checking queue name usage context..."
rg -C 3 "validation|generation" --type go
# Check configuration loading and usage
echo "Checking configuration usage..."
ast-grep --pattern 'type Config struct {
$$$
}'
# Look for NATS related code with more specific patterns
echo "Checking NATS queue operations..."
ast-grep --pattern 'nc.QueueSubscribe(conf.$_, $$$)'
ast-grep --pattern 'nc.Subscribe(conf.$_, $$$)'
ast-grep --pattern 'nc.Publish(conf.$_, $$$)'
Length of output: 2716
Script:
#!/bin/bash
# Check the consumer implementations
echo "Checking validate consumer implementation..."
rg -C 10 "QueueSubscribe|Subscribe" cmd/validate_consumer.go
echo "Checking generate consumer implementation..."
rg -C 10 "QueueSubscribe|Subscribe" cmd/generate_consumer.go
# Also check for any config loading in these files
echo "Checking how config is used in consumers..."
rg -C 3 "conf\." cmd/validate_consumer.go cmd/generate_consumer.go
Length of output: 2348
Makefile (3)
7-9: LGTM! Good addition of goimports
Adding goimports as a separate target and including it in the lint dependency chain will help maintain consistent import formatting.
18-19: Verify version bump impact
The Docker image version has been bumped from 0.0.10 to 0.0.11. Ensure this version change is reflected in deployment configurations.
1-2: Consider restoring the --enable-all flag for comprehensive linting
The removal of --enable-all from golangci-lint might reduce the scope of linting checks. While this could speed up the linting process, it might miss potential issues.
service/validator.go (1)
Line range hint 71-77: LGTM! Proper context cancellation handling
The context cancellation check is correctly implemented and aligns with the PR objective of improving error handling.
cmd/generate_consumer.go (1)
25-25: LGTM! Improved error handling with RunE
The switch from Run to RunE and the enhanced error group handling is a good improvement. The context cancellation check is properly implemented.
Also applies to: 70-77
cmd/validate_consumer.go (1)
25-25: Improved error handling with RunE and context cancellation
The switch to RunE and the new error handling logic for goroutine completion is a good improvement. The code now properly handles context cancellation and ensures errors are logged.
Also applies to: 72-79
Refactored consumer processes to improve error handling by wrapping deferred unsubscribes and separating logic into smaller methods. Updated workflows, Makefile, and Docker-related tags to version 0.0.12. Documentation and error logging enhancements were also added for better clarity and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/main.yml(4 hunks)Makefile(2 hunks)README.md(1 hunks)cmd/generate_consumer.go(4 hunks)cmd/validate_consumer.go(4 hunks)service/generator.go(3 hunks)service/validator.go(3 hunks)
🔇 Additional comments (23)
cmd/generate_consumer.go (3)
25-25: RunE transition is appropriate.
Switching from Run to RunE is a good practice because it lets you return errors directly and handle them in a consistent manner within Cobra commands.
60-60: Deferred unsubscribe with error handling.
Using defer func() { _ = sub.Unsubscribe() }() gracefully handles potential unsubscribe errors, preventing them from bubbling up unexpectedly.
70-77: Comprehensive error handling for goroutines.
The new error check:
- Returns
nilif there is no error or if the context was canceled. - Logs and returns other errors.
This is a robust pattern, ensuring you handle cancellations gracefully while still capturing and reporting unexpected failures.
cmd/validate_consumer.go (4)
4-7: Context and errors imports used effectively.
You've added context and errors to manage cancellation checks and custom error handling. This is a good practice for robust command execution.
25-25: Switch to RunE for better error propagation.
Similar to generate_consume, returning an error directly through RunE improves error handling logic and consistency across commands.
62-62: Deferred unsubscribe with error suppression.
Wrapping sub.Unsubscribe() in a closure that discards the returned error is acceptable when unsubscribing errors can be safely ignored.
72-79: Consistent goroutine error waiting pattern.
Checking for nil or context.Canceled ensures graceful stopping. Logging other errors provides visibility into unexpected issues.
service/validator.go (8)
5-5: Blank line usage.
This additional blank line could just be for readability. It's fine as is.
20-20: Value receiver consideration.
Changing from a pointer to a value receiver can be beneficial if the method doesn’t modify the receiver. Ensure there's no large struct copying or performance impact. For large structs, pointer receivers might be preferable.
31-33: Running sub-operation in a helper function.
Delegating logic to s.run(ctx, msg) is a clean approach. It promotes maintainability by separating higher-level control flow from detailed message processing.
35-39: Handle context cancellation gracefully.
Returning ctx.Err() under ctx.Done() is a clean pattern to signal a normal shutdown rather than an unexpected error.
41-64: Refactored run method improves error handling and logging.
Introducing a dedicated method reduces complexity in the main loop, adding structure to handle parsing, error responses, and logs. This is a strong approach to keep code readable and modular.
66-75: Efficient error response for validation failure.
Responding with dictionary.TypeValidatedError upon IsValid failure ensures consistent handling. Logging the underlying error is also helpful for observability.
77-80: Clear distinction between valid and invalid states.
Separating dictionary.TypeValid and dictionary.TypeInvalid is straightforward and easy to follow for consumers of this service.
82-93: Constructing and sending the reply message.
Including the message ID in the response header helps correlate logs with requests. The error handling on RespondMsg is also a good safety measure.
README.md (1)
3-3: Updated service description for clarity.
The more descriptive text accurately portrays the service’s features, indicating both validation and generation. This clarifies the scope of the microservice.
Makefile (2)
1-2: Lint target includes goimports now.
Adding goimports to the lint step automates import organization and keeps code style consistent.
7-9: New goimports target.
Defining a dedicated goimports target clarifies its usage, making it simpler to maintain formatting standards consistently across the codebase.
service/generator.go (4)
31-33: LGTM: Improved error handling structure.
The error handling has been improved by delegating processing to the run method and properly propagating errors.
35-38: LGTM: Proper context cancellation handling.
The implementation correctly handles context cancellation, aligning with the PR objective of improving error handling.
44-47: LGTM: Well-structured context enrichment.
The context is properly enriched with message ID for logging and tracing purposes.
Line range hint 20-39: Verify impact of changing receiver type from pointer to value.
The Process method's receiver has been changed from pointer (*Generator) to value (Generator). While this change is safe since Generator only contains a pointer field (passwordSvc), it's worth verifying all method calls in the codebase.
✅ Verification successful
Changing Generator receiver type from pointer to value is safe
The verification shows that the change from pointer to value receiver for the Process method is safe because:
- The only usage of
Generator.Processis incmd/generate_consumer.gowhere the Generator is created and its Process method is called immediately:service.NewGenerator(service.NewPasswordSvc(cfg)).Process(ctx, mch) - The Generator struct only contains a pointer field (
passwordSvc *Password), so copying the struct is efficient - There are no other methods or usages that could be affected by this change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Generator method calls to ensure compatibility
ast-grep --pattern 'generator := $_.NewGenerator($_)
$$$
generator.$_($_)'
Length of output: 76
Script:
#!/bin/bash
# Let's try different patterns to find Generator usage and instantiation
# Find files containing "Generator"
echo "=== Files containing Generator ==="
rg -l "Generator"
# Find Generator method calls and initialization
echo -e "\n=== Generator usage patterns ==="
rg -A 3 "Generator"
# Find specific Process method calls
echo -e "\n=== Process method calls ==="
rg -A 2 "\.Process\("
Length of output: 1682
.github/workflows/main.yml (1)
25-25: Verify Go version compatibility.
The Go version has been updated to 1.23.4. Ensure this version is consistent across:
- go.mod file
- Dockerfile
- Other CI/CD configurations
Also applies to: 52-52
✅ Verification successful
Go version 1.23.4 is consistently used across the codebase
The verification shows that Go version 1.23.4 is consistently used in:
go.mod:go 1.23.4Dockerfile:FROM golang:1.23.4-alpine3.21- GitHub Actions workflow:
go-version: 1.23.4
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Go version consistency
echo "Checking go.mod version:"
rg -g "go.mod" "^go \d+\.\d+(\.\d+)?"
echo "Checking Dockerfile Go version:"
rg -g "Dockerfile" "FROM golang:\d+\.\d+(\.\d+)?"
echo "Checking other CI configs:"
fd -g "*.yml" -g "*.yaml" | xargs rg "go-version: \d+\.\d+(\.\d+)?"
Length of output: 515
| docker build . -t soulgarden/swup:pass-0.0.12 --platform linux/amd64 | ||
| docker push soulgarden/swup:pass-0.0.12 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Version bump for Docker image.
Incrementing from pass-0.0.10 to pass-0.0.12 aligns with ongoing changes. Ensure any relevant deployment scripts or references are updated accordingly.
| reply := nats.NewMsg("") | ||
|
|
||
| reply.Header.Set("id", msgID) | ||
| reply.Header.Set("type", dictionary.TypeGenerated) | ||
| reply.Data = body | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding response timeout handling.
The response construction and sending could benefit from timeout handling to prevent hanging in case of slow consumers.
Consider wrapping the response in a context with timeout:
+ ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+ defer cancel()
reply := nats.NewMsg("")
reply.Header.Set("id", msgID)
reply.Header.Set("type", dictionary.TypeGenerated)
reply.Data = body
- if err := msg.RespondMsg(reply); err != nil {
+ if err := msg.RespondMsgWithContext(ctx, reply); err != nil {Committable suggestion skipped: line range outside the PR's diff.
| hash, salt, err := s.passwordSvc.HashPassword(ctx, in.Password) | ||
| if err != nil { | ||
| zerolog.Ctx(ctx).Err(err).Msg("hash password") | ||
|
|
||
| if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { | ||
| zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") | ||
| if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { | ||
| zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") | ||
|
|
||
| return err | ||
| } | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| reply := nats.NewMsg("") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling behavior.
When password hashing fails, the method returns nil after sending the error response, while other error cases return the error. This inconsistency could lead to silent failures.
Apply this fix:
if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil {
zerolog.Ctx(ctx).Err(err).Msg("nats queue respond")
return err
}
- return nil
+ return err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hash, salt, err := s.passwordSvc.HashPassword(ctx, in.Password) | |
| if err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("hash password") | |
| if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") | |
| if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") | |
| return err | |
| } | |
| } | |
| return err | |
| } | |
| reply := nats.NewMsg("") | |
| return nil | |
| } | |
| hash, salt, err := s.passwordSvc.HashPassword(ctx, in.Password) | |
| if err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("hash password") | |
| if err := msg.Respond([]byte(dictionary.TypeGeneratedError)); err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") | |
| return err | |
| } | |
| return err | |
| } |
| file: Dockerfile | ||
| push: false | ||
| tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.10 | ||
| tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.12 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider using semantic versioning for Docker tags.
The Docker image tag has been updated to pass-0.0.12, but consider also adding :latest tag for convenience.
tags: |
${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.12
+ ${{ secrets.DOCKER_HUB_USERNAME }}/swup:latestAlso applies to: 98-98
1 participant
Updated Go version to 1.23.4 across workflows, Dockerfile, and
go.mod. Enhanced error handling in consumers by switching toRunEand checking for context cancellation. Adjusted NATS queue defaults and removed unused configuration keys for cleaner setup.Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation