fix: Check if token is expired#49
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds logic to skip unnecessary GitHub token regeneration when the existing token is still valid. The controller now checks if a managed secret's token is still valid (based on expiry time and refresh interval) before generating a new installation token, improving efficiency by avoiding redundant API calls to GitHub.
Key Changes:
- Added token validity check before regeneration in the reconciliation loop
- Returns early with appropriate requeue timing when token is still valid
- Added comprehensive test coverage for the new token validity check behavior
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
controllers/gitrepository_controller.go |
Implements token validity check logic to skip regeneration when token hasn't expired beyond refresh threshold |
controllers/gitrepository_controller_test.go |
Adds test case verifying that reconciliation skips token regeneration when existing token is valid |
| // Check if existing secret has a valid token | ||
| existingSecret, err := r.secretManager.GetSecret(ctx, secretNamespace, secretName) | ||
| if err == nil && r.secretManager.IsSecretManagedByController(existingSecret) { | ||
| expiry, err := r.secretManager.GetTokenExpiry(existingSecret) | ||
| if err == nil { | ||
| refreshThreshold := r.Config.TokenRefresh.RefreshInterval | ||
| if time.Until(expiry) > refreshThreshold { | ||
| logger.V(1).Info("Token still valid, skipping regeneration", "expiresAt", expiry) | ||
| // Schedule token refresh as usual | ||
| if err := r.refreshManager.ScheduleRefresh(ctx, secretNamespace, secretName, gitRepo.Spec.URL); err != nil { | ||
| logger.Error(err, "Failed to schedule token refresh") | ||
| } | ||
| return ctrl.Result{RequeueAfter: time.Until(expiry) - 5*time.Minute}, nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The variable name 'err' is shadowed three times in nested scopes (lines 102, 104, 110), which reduces code clarity and makes it harder to track which error is being referenced. Consider using distinct variable names like 'getSecretErr', 'expiryErr', and 'scheduleErr' for better readability.
No description provided.