Skip to content
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 setting Activity Log enable flag through the API #10594

Merged
merged 6 commits into from
Dec 18, 2020

Conversation

swayne275
Copy link
Contributor

This is a follow-up to #10485

  • somewhere along the way we lost the actual line that sets enabled from the config update
  • updated tests to log warning(s) rather than fail (@mgritter is this in line with the spirit of these tests?)
  • cleaned up a little bit

Note: I didn't add a test for handleActivityConfigUpdate in logical_system_test.go because:

  • Currently we don't test this function in that file
  • the code is covered by stuff in at least activity_log_test.go
  • the activity_log_test.go tests caught this bug

activity_log_test.go tests are passing locally now

Comment on lines +169 to +173
func (a *ActivityLog) GetEnabled() bool {
a.fragmentLock.RLock()
defer a.fragmentLock.RUnlock()
return a.enabled
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh... I told Bez we should just add this so we could check GetEnabled() before and after SetConfig.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so this exists in an _ent file but not OSS so I just moved it over

@swayne275 swayne275 merged commit 077225f into master Dec 18, 2020
@swayne275 swayne275 deleted the fix-activity-log-enable-set branch December 18, 2020 18:20
mgritter pushed a commit that referenced this pull request Jan 22, 2021
* core: add warning when disabling activity (#10485)
* Fix setting Activity Log enable flag through the API (#10594)
Co-authored-by: swayne275 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants