-
Notifications
You must be signed in to change notification settings - Fork 84
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
DAO & Mock Registry #7
Conversation
eriknelson
commented
Jan 26, 2017
- Implemented some basic dao features for talking with etcd
- Filled out a mock-registry as a separate binary so we have something to pull data from in a real way.
* DevRegistry implementation
r.config = config | ||
fmt.Printf("DevRegistry::Init with url -> [ %s ] \n", r.config.Url) | ||
r.log = log | ||
return nil |
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.
Why return anything if we're just going to return nil?
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.
Won't compile without it
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 expect real registries to be doing more interesting things with failure possibility; this guys just kind of a test stub for us for now.
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.
Oh I get it now. The interface signature requires an error for REAL registries. For the dev one we don't care. Ok I forgot about that. +1
|
||
return reg | ||
return reg, err |
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.
Looks good.
return nil | ||
func (r *RHCCRegistry) LoadApps() ([]*Spec, error) { | ||
r.log.Debug("RHCCRegistry::LoadApps") | ||
return []*Spec{}, nil | ||
} |
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 do like the pluggable registry mechanism.
fmt.Println("============================================================") | ||
fmt.Println("== Starting Ansible Service Broker... ==") | ||
fmt.Println("============================================================") | ||
|
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.
NICE formatting. 🎉 🏆 🏅
return app | ||
} | ||
|
||
func (a *App) Start() { | ||
a.log.Info("Starting application") | ||
a.log.Notice("Ansible Service Broker Started") | ||
a.registry.LoadApps() |
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.
What if this fails?
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.
Good catch; I actually should remove this line for this PR. I'm picturing this start method being the one responsibly for starting the http mux listener. That will call into methods like broker.Bootstrap()
(not written yet), which in turn, will be the guy that calls registry.LoadApps()
.
So the full story has yet to be written here. This PR should be a lot of support without action in Start()
. I'll try to make a note of this in //TODOs
to explain myself a little better inline.
// Key generators | ||
//////////////////////////////////////////////////////////// | ||
func specKey(id string) string { | ||
return fmt.Sprintf("/spec/%s", 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.
This file looks good too.
* Remove `LoadApps` call from App::Start. It doesn't belong there and got committed as part of testing. * Renamed `LoadApps` to `LoadSpecs` since it's more accurate
ACK |
* Add etcd client deps * Initial checkin of dao spec ops * Add mock-registry * Start loading Specs from the mock registry * DevRegistry implementation * Updates based on feedback * Remove `LoadApps` call from App::Start. It doesn't belong there and got committed as part of testing. * Renamed `LoadApps` to `LoadSpecs` since it's more accurate
…ft#54) * moves the methods for extracting credentials to runtime package * adds tests for extract credentials. This is better set up to be overriden in the future. This PR does not allow for the ExtractCredentials to be overridden.