-
Notifications
You must be signed in to change notification settings - Fork 3
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
Fix sentinel refresh and nil pointer issue #77
Conversation
@@ -76,6 +76,7 @@ func (processor *AppConfigurationProviderProcessor) processFullReconciliation() | |||
processor.RefreshOptions.ConfigMapSettingPopulated = true | |||
processor.RefreshOptions.updatedKeyValueETags = updatedSettings.KeyValueETags | |||
processor.RefreshOptions.updatedFeatureFlagETags = updatedSettings.FeatureFlagETags | |||
processor.RefreshOptions.updatedSentinelETags = updatedSettings.SentinelETags |
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.
Shall we check if the returned updatedSettings.SentinelETags
is not nil, then we also change the processor.RefreshOptions.sentinelChanged=true
? Then we don't have to add additional check in Finish()
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.
By doing this we can also ensure that the processor.ReconciliationState.SentinelETags
would never be set to nil
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.
There may be 2 scenarios when shouldReconcile is true. User adds monitoring section or removes it. If no sentinel is setup, we need to make sure the ReconciliationState.SentinelETags
be updated to empty list.
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.
I agree that sentinelETags should not be nil.
// Initialize the updatedETags with the current eTags | ||
refreshedETags[sentinel] = eTag | ||
} | ||
for sentinel, currentETag := range eTags { |
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 need to add a nil check for the eTags
?
* fix sentinel refresh and nil pointer * update * avoid return nil sentinelETags
No description provided.