-
Notifications
You must be signed in to change notification settings - Fork 314
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
feat: added flags for event audit #3859
Conversation
12d3bdb
to
d517051
Compare
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.
eventAuditEnabled
needs to be a map[string]bool
protected by configSubscriberLock
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3859 +/- ##
==========================================
- Coverage 69.21% 69.21% -0.01%
==========================================
Files 357 357
Lines 53132 53145 +13
==========================================
+ Hits 36776 36782 +6
- Misses 14041 14046 +5
- Partials 2315 2317 +2
☔ View full report in Codecov by Sentry. |
@@ -121,7 +120,8 @@ func (c *ConfigT) DestinationsMap() map[string]*DestinationT { | |||
} | |||
|
|||
type Settings struct { | |||
DataRetention DataRetention `json:"dataRetention"` | |||
DataRetention DataRetention `json:"dataRetention"` | |||
EventAuditEnabled bool `json:"eventAuditEnabled"` |
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.
Have we verified that data is unmarshalling into this struct automatically ?
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.
yes
backend-config/backend-config.go
Outdated
@@ -156,6 +156,8 @@ func filterProcessorEnabledDestinations(config ConfigT) ConfigT { | |||
source.Destinations = destinations | |||
modifiedConfig.Sources = append(modifiedConfig.Sources, source) | |||
} | |||
modifiedConfig.Settings.DataRetention.DisableReportingPII = config.Settings.DataRetention.DisableReportingPII |
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 use the whole Settings
struct instead of selectively picking fields??
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.
DataRetention has other properties that will get overridden, thats why didn't do this
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.
Not including workspace settings in the TopicProcessConfig
looks like a bug to me, cc @cisse21, @BonapartePC
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.
Yeah true.... this looks like a bug which we had.
Description
Fetching event audit enabled flag from workspace config and using it in processor to send events to esch_jobs table
Linear Ticket
https://linear.app/rudderstack/issue/DAT-285/add-ability-for-customer-to-self-serve-the-enablement-disablement-of
Security