Skip to content

refactor(manager): small refactors around the manager and token logic #10

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

refactor(manager): small refactors around the manager and token logic #10

wants to merge 5 commits into from

Conversation

ndyakov
Copy link
Member

@ndyakov ndyakov commented May 30, 2025

  • it is safer to set the receivedAt when creating the token
  • make sure the manager main loop won't panic

ndyakov added 3 commits May 30, 2025 19:32
@ndyakov
fix(manager): Improve channel closure handling
a83b5f9 
- Updated IsClosed function to accurately check if a channel is closed without consuming data unless necessary.
- Safely close the closedChan only if it is not already closed to avoid potential panics.
@ndyakov
fix(manager): Add panic recovery in token manager goroutine
95b59c5 
- Implemented panic recovery in the Start method of entraidTokenManager to prevent crashes and ensure listener is notified of errors.
@ndyakov
fix(token): Enhance token creation logic and documentation
776abb7 
- Updated the New function to return nil if expiresOn is zero to prevent panic.
- Added logic to set receivedAt to the current time and recalculate TTL if receivedAt is zero.
- Improved documentation to clarify the responsibilities of the caller regarding token validity and behavior when parameters are zero.
ndyakov
ndyakov commented May 31, 2025
View reviewed changes
ndyakov
ndyakov commented May 31, 2025
View reviewed changes
ndyakov
ndyakov commented May 31, 2025
View reviewed changes
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 2, 2025

Merging this branch will decrease overall coverage

Impacted Packages Coverage Δ 🤖
github.com/redis/go-redis-entraid/internal 100.00% (ø)
github.com/redis/go-redis-entraid/manager 96.28% (-2.07%) 👎
github.com/redis/go-redis-entraid/token 100.00% (ø)
Coverage by file

Changed files (no unit tests)

Changed File Coverage Δ Total Covered Missed 🤖
github.com/redis/go-redis-entraid/internal/utils.go 100.00% (ø) 3 3 0
github.com/redis/go-redis-entraid/manager/entraid_manager.go 93.75% (-3.39%) 112 (+7) 105 (+3) 7 (+4) 👎
github.com/redis/go-redis-entraid/token/token.go 100.00% (ø) 16 (+2) 16 (+2) 0

Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code.

Changed unit test files

  • github.com/redis/go-redis-entraid/manager/entraid_manager_test.go
  • github.com/redis/go-redis-entraid/token/token_test.go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ndyakov