Change registration to be dynamic rather than static #236
Conversation
This allows for resources to be defined outside of the rnode package
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| builderFuncs = map[string]newBuilderFunc{} | ||
| ) | ||
|
|
||
| // RegisterBuilder constructor. shortName is the plural lowerCamelCase resource |
There was a problem hiding this comment.
RegisterBuilder is not a constructor. The global variables are already created and this function is adding ("registering") the values so they can be used by rGraph. Can you change a bit the description?
There was a problem hiding this comment.
The sentence is supposed to be "Register Builder constructor." but this is not allowed by go.. I will clarify by expanding the description.
| type newBuilderFunc func(id *cloud.ResourceID) rnode.Builder | ||
|
|
||
| var ( | ||
| builderFuncsLock sync.Mutex |
There was a problem hiding this comment.
I think it will be useful ti use RWLock. Write will happened only in init. Then in rGraph we are reading the values from the map so RLock will be more efficient.
| alpha "google.golang.org/api/compute/v0.alpha" | ||
| beta "google.golang.org/api/compute/v0.beta" | ||
| "google.golang.org/api/compute/v1" | ||
| ) | ||
|
|
||
| func init() { all.RegisterBuilder("backendServices", NewBuilder) } |
There was a problem hiding this comment.
please extract "backendServices" to a const so we can use the value from the package and make sure that no typo will be introduced in NewBuilderByID. Please apply this comment to all cases.
| @@ -31,6 +32,8 @@ const ( | |||
| resourceName = "TcpRoute" | |||
| ) | |||
|
|
|||
| func init() { all.RegisterBuilder("tcpRoute", NewBuilder) } | |||
There was a problem hiding this comment.
should be plural tcpRoute -> tcpRoutes
This allows for resources to be defined outside of the rnode package