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

Bug 1504250 - Keep listening for deprovision messages #508

Merged
merged 1 commit into from Oct 26, 2017

Conversation

djzager
Copy link
Member

@djzager djzager commented Oct 19, 2017

Describe what this PR does and why we need it:

We want the subscriber to keep listening for deprovision messages even if a previous deprovision fails.

See https://bugzilla.redhat.com/show_bug.cgi?id=1504250 for recreation steps.

Before this change 'deprovision complete msg' would be sent to the channel but there would not be a corresponding 'processed deprovision message from buffer'.

After this change (from broker logs):

...
[2017-10-19T20:07:53.008Z] [DEBUG] sending deprovision complete msg to channel
[2017-10-19T20:07:53.008Z] [DEBUG] Processed deprovision message from buffer

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 19, 2017
@djzager djzager added bug needs-review and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 19, 2017
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

The problem is we have return statements in the for loop of the go routine. Simply change those to continue. That is why making them a method worked for you because the return brought you back to the loop. I should've seen this in the deprovision review.

@jmrodri
Copy link
Contributor

jmrodri commented Oct 19, 2017

OR chain the if statements so they only do one of them at a time and no need for continue or return. See the provision subscriber. Either way is acceptable.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 20, 2017
Copy link
Contributor

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

Um does not compile:

$ make build
go build -i -ldflags="-s -w" ./cmd/broker
# github.com/openshift/ansible-service-broker/pkg/broker
pkg/broker/deprovision_subscriber.go:60: undefined: log in log.Errorf
make: *** [Makefile:19: broker] Error 2

@djzager
Copy link
Member Author

djzager commented Oct 26, 2017

$ make build
go build -i -ldflags="-s -w" ./cmd/broker

$ make check
ok      github.com/openshift/ansible-service-broker/pkg/apb     0.190s
ok      github.com/openshift/ansible-service-broker/pkg/app     0.236s
ok      github.com/openshift/ansible-service-broker/pkg/auth    0.016s
ok      github.com/openshift/ansible-service-broker/pkg/broker  0.207s
?       github.com/openshift/ansible-service-broker/pkg/clients [no test files]
?       github.com/openshift/ansible-service-broker/pkg/dao     [no test files]
?       github.com/openshift/ansible-service-broker/pkg/fusortest       [no test files]
ok      github.com/openshift/ansible-service-broker/pkg/handler 0.309s
?       github.com/openshift/ansible-service-broker/pkg/metrics [no test files]
?       github.com/openshift/ansible-service-broker/pkg/origin/copy/authorization       [no test files]
ok      github.com/openshift/ansible-service-broker/pkg/origin/copy/authorization/validation    0.086s
?       github.com/openshift/ansible-service-broker/pkg/origin/copy/user        [no test files]
ok      github.com/openshift/ansible-service-broker/pkg/origin/copy/user/validation     0.081s
ok      github.com/openshift/ansible-service-broker/pkg/registries      0.079s
ok      github.com/openshift/ansible-service-broker/pkg/registries/adapters     0.102s
?       github.com/openshift/ansible-service-broker/pkg/runtime [no test files]
?       github.com/openshift/ansible-service-broker/pkg/version [no test files]

@rthallisey rthallisey merged commit dd6e7be into openshift:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants