-
Notifications
You must be signed in to change notification settings - Fork 578
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
Broker Ingress Tracing #1290
Broker Ingress Tracing #1290
Changes from 17 commits
fd3f55a
22963ff
d28a54b
8d10538
8c051e4
e053fe5
dc2c77e
2078cee
f59d651
40bd59a
a7846f6
f69b6fa
ba4d330
73de49e
45da8a8
3c54d83
f20d431
ee0e3bf
6c9113d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/* | ||
Copyright 2019 The Knative Authors | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package configmap | ||
Harwayne marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
import ( | ||
"github.com/knative/pkg/configmap" | ||
v1 "k8s.io/api/core/v1" | ||
) | ||
|
||
// TODO Move this to knative/pkg. | ||
|
||
// DefaultConstructors is a map for specifying default ConfigMaps to their function constructors. | ||
// | ||
// The values of this map must be functions with the definition: | ||
// | ||
// func(*k8s.io/api/core/v1.ConfigMap) (... , error) | ||
// | ||
// These functions can return any type along with an error. | ||
type DefaultConstructors map[*v1.ConfigMap]interface{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this a map? What are the semantics of a map keyed by struct pointers? I think this would make more sense to me as: type DefaultConstructorFunc func() *v1.ConfigMap
type DefaultConstructors map[string]DefaultConstructorFunc I think that would simplify this implementation since there's no need to store default configmaps. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is based on knative/pkg's configmap.Constructor: type Constructors map[string]interface{} The suggestion drops the 'constructor' portion of this map. Essentially, what I am doing is changing the key from the name of the ConfigMap, to the entire ConfigMap. As for pointer as key semantics, "Pointer values are comparable. Two pointer values are equal if they point to the same variable or if both have value nil. Pointers to distinct zero-size variables may or may not be equal.". So essentially, the pointer must be to the same place. The equality of what is being pointed at does not matter. I tried to use non-pointers, but v1.ConfigMap's are "invalid map key type". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't understand why this needs to be a map with a pointer key. This would be equivalent, right? type DefaultConstructor struct {
ConfigMap *v1.ConfigMap
Constructor func(*v1.ConfigMap) (interface{}, error)
}
func NewDefaultUntypedStore(..., defaultConstructors []DefaultConstructor, ...) I still like my version above better, but the array of structs makes more sense to me than the map. WDYT @Harwayne? Do you like pkg's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switched to slice of structs. The reason I was using a map to begin with was to protect against the same thing being registered twice. But that protection didn't actually occur. Both because it was a pointer and because I really wanted the key to just be the name of the ConfigMap, not its whole value. |
||
|
||
// DefaultUntypedStore is an UntypedStore with default values for ConfigMaps that do not exist. | ||
type DefaultUntypedStore struct { | ||
store *configmap.UntypedStore | ||
defaultCMs []v1.ConfigMap | ||
} | ||
|
||
// NewDefaultUntypedStore creates a new DefaultUntypedStore. | ||
func NewDefaultUntypedStore( | ||
name string, | ||
logger configmap.Logger, | ||
defaultConstructors DefaultConstructors, | ||
onAfterStore ...func(name string, value interface{})) *DefaultUntypedStore { | ||
constructors := configmap.Constructors{} | ||
defaultCMs := make([]v1.ConfigMap, 0, len(defaultConstructors)) | ||
for cm, ctor := range defaultConstructors { | ||
constructors[cm.Name] = ctor | ||
defaultCMs = append(defaultCMs, *cm) | ||
} | ||
return &DefaultUntypedStore{ | ||
store: configmap.NewUntypedStore(name, logger, constructors, onAfterStore...), | ||
defaultCMs: defaultCMs, | ||
} | ||
} | ||
|
||
// WatchConfigs uses the provided configmap.DefaultingWatcher to setup watches for the config maps | ||
// provided in defaultCMs. | ||
func (s *DefaultUntypedStore) WatchConfigs(w configmap.DefaultingWatcher) { | ||
for _, cm := range s.defaultCMs { | ||
w.WatchWithDefault(cm, s.store.OnConfigChanged) | ||
} | ||
} |
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 think I'd like this to be a function so it can be unit tested.
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.
Done. Even added a test :)