Dgromov/update handlers #69
Conversation
| @@ -39,6 +44,8 @@ imports: | |||
| - process | |||
| - time | |||
| - watch | |||
| - name: github.com/pborman/uuid | |||
There was a problem hiding this comment.
Ping this in glide.yaml?
| ) | ||
|
|
||
| // Namespaces returns the version and the persisted namespaces in kv store. | ||
| // Namespaces returns the version ad the persisted namespaces in kv store. |
| @@ -86,3 +89,97 @@ func Namespace(namespaces *schema.Namespaces, namespaceName string) (*schema.Nam | |||
|
|
|||
| return namespace, nil | |||
| } | |||
|
|
|||
| // CreateNamespace creates a blank namespace with a given name. | |||
| func CreateNamespace(store kv.TxnStore, | |||
There was a problem hiding this comment.
nit: I usually prefer to format function declarations as
func CreateNamespace(
store kv.TxnStore,
namespaceKey, namespaceName, ruleSetKey string,
propagationDelay time.Duration,
) error {
...
}
feels a bit easier to read
| // CreateNamespace creates a blank namespace with a given name. | ||
| func CreateNamespace(store kv.TxnStore, | ||
| namespaceKey, namespaceName, ruleSetKey string, | ||
| propDelay time.Duration) error { |
There was a problem hiding this comment.
It might make sense to make these standalone methods member functions of a handler struct so you don't need to always pass store or propDelay around, instead they'll be stored in the handler object after creation, e.g., something like
type HandlerOptions struct {
PropagationDelay time.Duration
}
type Handler struct {
store kv.TxnStore
opts HandlerOptions
...
}
func NewHandler(store kv.TxnStore, opts HandlerOptions) Handler {
return Handler{store: store, opts: opts}
}
func (h Handler) CreateNamespace(key, name, ruleSetKey string) error {
// use h.store and h.opts.PropagationDelay here
...
}
| if err != nil { | ||
| if err == errMultipleMatches { | ||
| return kv.ErrAlreadyExists | ||
| } else if err != kv.ErrNotFound { |
There was a problem hiding this comment.
nit: a bit cleaner and less nesting to write
if err == errMultipleMatches {
return kv.ErrAlreadyExists
}
if err != nil && err != kv.ErrNotFound {
return err
}
|
|
||
| // A rule with no snapshots should not be added. | ||
| if m.Snapshots == nil { | ||
| continue |
There was a problem hiding this comment.
This should be an error. Also if you are already checking the length below, you don't need this.
|
|
||
| ls := len(m.Snapshots) | ||
| if ls == 0 { | ||
| continue |
There was a problem hiding this comment.
Should probably be an error.
| continue | ||
| } | ||
|
|
||
| lastSnap := m.Snapshots[ls-1] |
There was a problem hiding this comment.
lastSnapshot is probably a better name.
| } | ||
| } | ||
|
|
||
| for _, r := range diff.RollupRules { |
There was a problem hiding this comment.
nit: this function is pretty long. Might make sense to break this function into two smaller functions applyMappingRuleChanges and applyRollupRuleChanges.
| } | ||
|
|
||
| // AppendToRuleSet appends a ruleset diff onto the end of a ruleset | ||
| func appendToRuleSet(orig, diff *schema.RuleSet) error { |
There was a problem hiding this comment.
applyRuleChanges might be a better name.
|
Closing this PR in favor of #72 |
No description provided.