Skip to content
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

Template service broker documentation and logging updates #14318

Merged

Conversation

jim-minter
Copy link
Contributor

Documentation and logging updates as a follow-up to #14216

@openshift/devex ptal

@jim-minter jim-minter force-pushed the trello132-broker-sar-updates branch 2 times, most recently from 719f213 to ee9ee8e Compare May 24, 2017 11:37
@@ -244,6 +287,9 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem
return errs[0]
}

// We add an OwnerReference to all objects we create - this is also needed
// by the template service broker for cleanup. TODO: what about any objects
// created by the templateinstance in other namespaces?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great question :/

func (b *Broker) Bind(instanceID, bindingID string, breq *api.BindRequest) *api.Response {
glog.V(4).Infof("Template service broker: Bind")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print the instanceid and binding id?

func (b *Broker) Catalog() *api.Response {
glog.V(4).Infof("Template service broker: Catalog")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe at least print the list of namespaces that it's fetching templates from. this will help when someone says "but it's not retrieving all my templates"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have added this to the service broker startup log message - unless you really want it logged on every time a catalog call is made (and loglevel >= 4)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's fine.

// TODO: currently the spec does not allow for user information to be
// provided on Deprovision, so little authorization can be carried out.

glog.V(4).Infof("Template service broker: Deprovision")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print the instanceid

// TODO: currently the spec does not allow for user information to be
// provided on LastOperation, so little authorization can be carried out.

glog.V(4).Infof("Template service broker: LastOperation")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instance id

func (b *Broker) Provision(instanceID string, preq *api.ProvisionRequest) *api.Response {
glog.V(4).Infof("Template service broker: Provision")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instanceid

func (b *Broker) Unbind(instanceID, bindingID string) *api.Response {
glog.V(4).Infof("Template service broker: Unbind")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instance/bindingid

@bparees
Copy link
Contributor

bparees commented May 24, 2017

couple nits related to what's being logged but mostly looks great. The readme is especially helpful imho.

@jim-minter jim-minter force-pushed the trello132-broker-sar-updates branch from ee9ee8e to 2593143 Compare May 25, 2017 13:41
@jim-minter
Copy link
Contributor Author

@bparees updated and pushed

@bparees
Copy link
Contributor

bparees commented May 25, 2017

[merge][severity:bug]

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017
@jim-minter jim-minter force-pushed the trello132-broker-sar-updates branch from 2593143 to 3b0f19f Compare May 26, 2017 08:42
@jim-minter
Copy link
Contributor Author

rebased, please remerge

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 3b0f19f

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2017
@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1791/) (Base Commit: a1dffba)

@jim-minter
Copy link
Contributor Author

flake #13980, please remerge

@bparees
Copy link
Contributor

bparees commented May 26, 2017

[merge][severity:bug]

1 similar comment
@bparees
Copy link
Contributor

bparees commented May 27, 2017

[merge][severity:bug]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 3b0f19f

@openshift-bot
Copy link
Contributor

openshift-bot commented May 27, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/829/) (Base Commit: 060f5e5) (Extended Tests: bug) (Image: devenv-rhel7_6280)

@openshift-bot openshift-bot merged commit 703a524 into openshift:master May 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants