Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

fix invalid log #2429

Merged
merged 2 commits into from
Oct 23, 2018
Merged

fix invalid log #2429

merged 2 commits into from
Oct 23, 2018

Conversation

carlory
Copy link
Contributor

@carlory carlory commented Oct 21, 2018

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2018
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 21, 2018
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

Change looks good to me as far as the correctness of the change.

/hold
for @duglin to add opinion.

pkg/controller/broker_client_manager.go Show resolved Hide resolved
@@ -96,7 +97,7 @@ func (m *BrokerClientManager) RemoveBrokerClient(brokerKey BrokerKey) {
m.mu.Lock()
defer m.mu.Unlock()

glog.V(4).Info("Removing OSB client for broker %s", brokerKey.String())
glog.V(4).Infof("Removing OSB client for broker %s", brokerKey.String())
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a correct change. Format string wasn't being used as one.

@duglin do you want a %q?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. I also think %q is better than %s. If @duglin want a %q, I will change them in the go file.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes %q please

Copy link
Contributor

Choose a reason for hiding this comment

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

yes %q please

Copy link
Contributor

Choose a reason for hiding this comment

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

yes %q please

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, please use a %q

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, please use a %q

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please use %q

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please use %q

Copy link
Contributor

Choose a reason for hiding this comment

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

yes please use %q

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 21, 2018
@carlory
Copy link
Contributor Author

carlory commented Oct 22, 2018

@MHBauer @duglin I have changed them. Please review again !

1 similar comment
@carlory
Copy link
Contributor Author

carlory commented Oct 22, 2018

@MHBauer @duglin I have changed them. Please review again !

@luksa
Copy link
Contributor

luksa commented Oct 22, 2018

/hold cancel
/ok-to-test
/lgtm

@luksa
Copy link
Contributor

luksa commented Oct 22, 2018

/ok-to-test
/lgtm
/hold cancel

@duglin
Copy link
Contributor

duglin commented Oct 22, 2018 via email

@k8s-ci-robot k8s-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 22, 2018
@carlory
Copy link
Contributor Author

carlory commented Oct 23, 2018

/test pull-service-catalog-xbuild

@jboyd01
Copy link
Contributor

jboyd01 commented Oct 23, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jboyd01

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit d1fed3a into kubernetes-retired:master Oct 23, 2018
@carlory carlory deleted the fix-log branch October 24, 2018 11:03
jboyd01 pushed a commit to jboyd01/service-catalog that referenced this pull request Oct 31, 2018
* fix invalid log

* use `%q`
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants