-
Notifications
You must be signed in to change notification settings - Fork 127
Draft: CSPL-4344 #1679
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: develop
Are you sure you want to change the base?
Draft: CSPL-4344 #1679
Conversation
|
CLA Assistant Lite bot CLA Assistant Lite bot All contributors have signed the COC ✍️ ✅ |
5c684e8 to
bf5a91d
Compare
| if licenseInfo.Status == "EXPIRED" { | ||
| eventPublisher.Warning(ctx, "LicenseExpired", | ||
| fmt.Sprintf("License '%s' has expired", licenseName)) | ||
| scopedLog.Info("Detected expired license", "licenseName", licenseName, "title", licenseInfo.Title) |
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.
Should it be Info?
Pull Request Test Coverage Report for Build 21901106831Details
💛 - Coveralls |
| var pod corev1.Pod | ||
| err := client.Get(ctx, namespacedName, &pod) | ||
| if err != nil { | ||
| // Pod might not exist yet, which is normal during initial creation |
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.
Shouldn't we log something on return?
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.
Those tests look like they could be a table tests.
Which API call will fail in case of expected alerts? Shouldn't we mock it? I would prefer to have some positive test case
b2c1599 to
43a33fe
Compare
|
|
||
| // checkLicenseRelatedPodFailures checks license status via Splunk API | ||
| // and publishes warning event when expired license is detected | ||
| func checkLicenseRelatedPodFailures(ctx context.Context, client splcommon.ControllerClient, cr *enterpriseApi.LicenseManager, statefulSet *appsv1.StatefulSet, eventPublisher *K8EventPublisher) { |
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.
Can't we just take the eventPublisher from the context?
| scopedLog := reqLogger.WithName("checkLicenseRelatedPodFailures") | ||
|
|
||
| // Check if pod is ready before attempting API call | ||
| podName := fmt.Sprintf("%s-0", statefulSet.GetName()) |
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.
Do we want to only check it for one pod?
| } | ||
|
|
||
| // Check for license-related pod failures before updating | ||
| checkLicenseRelatedPodFailures(ctx, client, cr, statefulSet, eventPublisher) |
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.
Don't we want to throw an error if sth goes wrong?
Description
What does this PR have in it?
Key Changes
Highlight the updates in specific files
Testing and Verification
How did you test these changes? What automated tests are added?
Related Issues
Jira tickets, GitHub issues, Support tickets...
PR Checklist