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

Jump to bottom

pass provision credentials during deprovision #821

Merged
merged 2 commits into from Mar 16, 2018
Merged

pass provision credentials during deprovision #821

merged 2 commits into from Mar 16, 2018

Conversation

maleck13
Copy link
Contributor

@maleck13 maleck13 commented Mar 7, 2018

Describe what this PR does and why we need it:

Changes proposed in this pull request

  • pass provision credentials during deprovision
  • add a make generate target for generating upto date mocks easily
  • change the logic in broker.go provision to check if the error is a not found error

Which issue this PR fixes (This will close that issue when PR gets merged)
fixes #818

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 7, 2018
maleck13
maleck13 commented Mar 7, 2018
View reviewed changes
pkg/broker/broker.go
log.Debug("already have this instance returning 200")
return &ProvisionResponse{}, ErrorAlreadyProvisioned
si, err := a.dao.GetServiceInstance(instanceUUID.String())
if err != nil && !a.dao.IsNotFoundError(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to avoid ignoring other errors

@maleck13 maleck13 mentioned this pull request Mar 7, 2018
Merged
@shawn-hurley
Copy link
Contributor

This is currently failing because certain params are not being passed to the playbook. They include _apb_plan_id. I think that we may some logic somewhere that sent these things if the params were empty.

@maleck13 can you take a look and see if that is the case?

@maleck13
Copy link
Contributor Author

maleck13 commented Mar 8, 2018 •
edited

@shawn-hurley Yes I will take a look thanks for the help. We were passing serviceinstance.params so likely I need to build up the params map in a similar way as we do in the provision method
Out of curiosity how did you figure this out? Find it difficult to parse the CI job on travis, are you running the ci job locally?

@maleck13
Copy link
Contributor Author

maleck13 commented Mar 9, 2018

This should be ok now.

deprovision from master shows

'{"_apb_plan_id":"default","_apb_service_class_id":"b43a4272a6efcaaa3e0b9616324f1099","_apb_service_instance_id":"aeede2b5-b474-4b85-90b2-fd2f278863bc","cluster":"openshift","namespace":"test-depro","postgresql_database":"admin","postgresql_password":"admin","postgresql_user":"admin"}'

deprovision from this branch shows

'{"_apb_plan_id":"default","_apb_provision_creds":{"DB_HOST":"hello-world-db","DB_NAME":"admin","DB_PASSWORD":"admin","DB_PORT":"5432","DB_TYPE":"postgres","DB_USER":"admin"},"_apb_service_class_id":"b43a4272a6efcaaa3e0b9616324f1099","_apb_service_instance_id":"343861e7-ce62-4587-a2b1-f36b0696f800","cluster":"openshift","namespace":"depro","postgresql_database":"admin","postgresql_password":"admin","postgresql_user":"admin"}'

@maleck13
Copy link
Contributor Author

@shawn-hurley I believe this change is good to go now

@jmrodri jmrodri self-requested a review March 14, 2018 02:29
jmrodri
jmrodri reviewed Mar 14, 2018
View reviewed changes
Makefile
@@ -23,6 +23,10 @@ broker: $(SOURCES) ## Build the broker
build: broker ## Build binary from source
@echo > /dev/null

generate: ## regenerate mocks
go get github.com/vektra/mockery/.../
@go generate ./...
Copy link
Contributor

Choose a reason for hiding this comment

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

I hate so many of the go tools, they totally don't work with symlinks. UGH! The minute I move to a real directory, it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will fix this after this PR gets merged. Looks like there might be a workaround to work with symlinks
vektra/mockery#157 (comment)

jmrodri
jmrodri reviewed Mar 14, 2018
View reviewed changes
Makefile
@@ -23,6 +23,10 @@ broker: $(SOURCES) ## Build the broker
build: broker ## Build binary from source
@echo > /dev/null

generate: ## regenerate mocks
go get github.com/vektra/mockery/.../
Copy link
Contributor

Choose a reason for hiding this comment

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

@maleck13 is there a reason why we didn't prepend the go get line with @?

jmrodri
jmrodri approved these changes Mar 14, 2018
View reviewed changes
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.

Thanks looks good.

eriknelson
eriknelson approved these changes Mar 16, 2018
View reviewed changes
Copy link
Contributor

@eriknelson eriknelson left a comment

Choose a reason for hiding this comment

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

Looks good @maleck13!

@shawn-hurley shawn-hurley merged commit 26b4c94 into openshift:master Mar 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@eriknelson eriknelson eriknelson approved these changes

@shawn-hurley shawn-hurley shawn-hurley approved these changes

Assignees
No one assigned
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Pass provision credentials to the deprovision playbook even if APB is not bindable
5 participants
@maleck13 @shawn-hurley @jmrodri @eriknelson @openshift-ci-robot