-
Notifications
You must be signed in to change notification settings - Fork 1
Merge critical IAM token refresh bug fix and test improvements from upstream #72
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
📋 API Contract Changes Summary✅ No API surface changes detected relative to main. This automated check examined exported types, functions, methods, and interfaces for the core framework and all modules. If you expected API changes, ensure exported identifiers have the correct casing and that the contract extraction tool supports the new patterns. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
modules/database/aws_iam_auth.go
Outdated
| if r := recover(); r != nil { | ||
| // Log the panic but don't fail the token refresh process | ||
| // The actual logging will be handled by the callback implementation if available | ||
| fmt.Printf("Database token refresh callback panic recovered: %v\n", r) |
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.
@copilot Use the logger service rather than resorting to printf.
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.
Fixed in commit a3e1dc5. The AWSIAMTokenProvider now accepts a logger parameter and uses the logger service instead of fmt.Printf for panic recovery reporting.
… recovery Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
This PR merges critical bug fixes and improvements from the CrisisTextLine upstream fork while preserving all GoCodeAlone enhancements and functionality.
Key Changes
🔧 IAM Token Refresh Bug Fix
Merged the critical IAM token refresh fix from upstream (commit 75708cc) that prevents database disconnection after AWS IAM token expiration:
TokenRefreshCallbackinterface: Enables automatic notification when IAM tokens are refreshedAWSIAMTokenProvider: Now includes token refresh callbacks with panic recovery and proper logger integrationonTokenRefreshmethod recreates database connections with new tokensConnectionTimeoutfield inAWSIAMAuthConfig🛡️ Nil Pointer Panic Protection
Verified that the nil pointer panic fix from upstream (commit 486711f) is already present and working in our codebase. All interface matching scenarios pass without panics.
🧪 Test Coverage Improvements
Added comprehensive test coverage for the new IAM token refresh functionality:
MockLoggerfor consistent testing across database module🔄 Database Module Enhancements
Updated the database module to support the new IAM token refresh functionality:
NewDatabaseServiceto accept logger parameterinitializeConnectionsto pass application loggerfmt.Printffor panic recoveryTesting
All tests pass including:
Example Usage
The IAM token refresh now works automatically:
This merge ensures users benefit from both the critical upstream bug fixes and all GoCodeAlone improvements, providing a robust and stable database connection experience with AWS IAM authentication.
Fixes #69.
Recent Updates
fmt.Printfwith logger service in IAM token refresh panic recoveryAWSIAMTokenProviderconstructor to accept logger parameter✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.