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

Add deregister command in svcat #2314

Merged

Conversation

jichenjc
Copy link
Contributor

svcat only support register now, deregister is added to support
remove broker registered.

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

luksa commented Aug 31, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 31, 2018
@luksa
Copy link
Contributor

luksa commented Aug 31, 2018

@jichenjc You need to run make test-update-goldenfiles.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 31, 2018
@jichenjc
Copy link
Contributor Author

I run make test golden configuration and encoutner weird issue locally, submit to CI for double check

@@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM golang:GO_VERSION
FROM golang:1.10.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you committed something you didn't want to :)

@luksa
Copy link
Contributor

luksa commented Aug 31, 2018

@jichenjc Yeah, I saw the same issue when I ran make locally, too. Looks like it's not a real issue at all.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2018
@luksa
Copy link
Contributor

luksa commented Aug 31, 2018

/lgtm cancel

Actually, @jichenjc can you add a deregister broker test to TestCommandOutput() in svcat_test.go. Make sure to include a newline at the end of output/deregister-broker.txt and you'll see that the command doesn't add the newline:

[luksa@w541 service-catalog]$ svcat deregister ups-broker
Successfully removed broker "ups-broker"[luksa@w541 service-catalog]$ 

Can you also please fix that? I'd fix it myself, but I can't until this PR is merged, so you might as well do it here.

@k8s-ci-robot k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2018
@jichenjc
Copy link
Contributor Author

jichenjc commented Sep 3, 2018

ok, I will fix it soon by adding the test and the new line, thanks for the reminder

@jichenjc
Copy link
Contributor Author

jichenjc commented Sep 3, 2018

root@kvm-016922:/go/src/github.com/kubernetes-incubator/service-catalog# bin/svcat/svcat get brokers
NAME NAMESPACE URL STATUS
+--------------+-----------+------------------------------------------------+----------------------+
nasty-broker http://nasty-service.default.svc.cluster.local ErrorFetchingCatalog
root@kvm-016922:
/go/src/github.com/kubernetes-incubator/service-catalog# bin/svcat/svcat deregister nasty-broker
Successfully removed broker "nasty-broker"

should be fine now :)

svcat only support register now, deregister is added to support
remove broker registered.
@luksa
Copy link
Contributor

luksa commented Sep 3, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2018
@jboyd01
Copy link
Contributor

jboyd01 commented Sep 4, 2018

This looks good for Cluster service brokers but lacks support & tests for Name Spaced brokers. We can do that in a followup. @jichenjc do you have any interest in doing that next? :)
/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 Sep 4, 2018
@k8s-ci-robot k8s-ci-robot merged commit cf01a6a into kubernetes-retired:master Sep 4, 2018
@jichenjc
Copy link
Contributor Author

jichenjc commented Sep 5, 2018

yes, I will create another issue and PR to fix the namespace followup, thanks for the reminder

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants