-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
Instead of relying on unmarshalling json, will now use Mapping and Rollup Views. These views represent a state of ruleset ready to be looked at by a human on a screen.
@@ -160,6 +131,40 @@ func (mrs *mappingRuleSnapshot) Schema() (*schema.MappingRuleSnapshot, error) { | |||
return res, nil | |||
} | |||
|
|||
// MappingRuleView is a human friendly representation of a mapping rule at a given point in time. | |||
type MappingRuleView struct { |
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.
are you wrapping the struct access w/ methods to ensure the fields are immutable?
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, but I kind of rather just make all the fields exposed. They can't be used for anything.
func newRollupTargetView(target RollupTarget) RollupTargetView { | ||
return RollupTargetView{ | ||
Name: string(target.Name), | ||
Tags: stringArrayFromBytesArray(target.Tags), |
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.
nit: technically it's a slice not an array
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.
That naming convention was established before I started touching this code base. I can change it but would like to do that in a separate pr if possible.
rules/rollup.go
Outdated
return RollupTarget{ | ||
Name: []byte(rtv.Name), | ||
Tags: bytesArrayFromStringArray(rtv.Tags), | ||
Policies: rtv.Policies} |
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.
nit: newline before }
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.
Cool
rules/ruleset.go
Outdated
return rollupRules | ||
} | ||
|
||
//UpdateMetadata contains descriptive information that needs to be updated |
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.
nit: space after //
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.
also move this outside of the sections where you define all the ruleSet methods? maybe closer to its constructor?
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.
Will do
rules/ruleset.go
Outdated
} | ||
rs.updateMetadata(meta) | ||
return nil | ||
} | ||
|
||
func (rs *ruleSet) DeleteMappingRule(d DeleteData) error { | ||
m, err := rs.getMappingRuleByID(d.ID) | ||
func (rs *ruleSet) DeleteMappingRule(ID string, meta UpdateMetadata) error { |
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.
s/ID/id ?
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.
Will do
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.
LGTM with a nit
rules/ruleset.go
Outdated
@@ -598,7 +612,7 @@ func (rs *ruleSet) ToMutableRuleSet() MutableRuleSet { | |||
return rs | |||
} | |||
|
|||
// Schema returns the protobuf representation fo a ruleset | |||
// Schema returns the protobuf representation fo a ruleset. |
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.
nit: s/fo/of?
Instead of relying on unmarshalling json, will now use Mapping and
Rollup Views. These views represent a state of ruleset ready to be
looked at by a human on a screen.