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

Re-implement V3 Service Bindings #1158

Merged
merged 1 commit into from
Jul 27, 2022
Merged

Re-implement V3 Service Bindings #1158

merged 1 commit into from
Jul 27, 2022

Conversation

radito3
Copy link
Contributor

@radito3 radito3 commented Jun 29, 2022

No description provided.

Copy link
Contributor

@dmikusa dmikusa left a comment

Choose a reason for hiding this comment

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

I took my first pass through this, and it looks good. There are no integration tests though. I'd like to see some basic coverage of the functionality in integration tests. It's one thing to test with mock responses, but we want to have test coverage that interacts with an actual CF environment too.

I know writing and running integration tests is not trivial. If you are able to put the tests together, I can run them in one of our labs.

@dmikusa dmikusa mentioned this pull request Jul 21, 2022
@radito3
Copy link
Contributor Author

radito3 commented Jul 25, 2022

Hi @dmikusa-pivotal ,
I wrote integration tests for V3 Service Bindings.
Could you take another look at the changes?

@dmikusa
Copy link
Contributor

dmikusa commented Jul 27, 2022

@radito3 I ran the integration tests and am getting an error, looks like the same one, across all the tests...

[INFO] Running org.cloudfoundry.client.v3.ServiceBindingsTest
[ERROR] Tests run: 7, Failures: 7, Errors: 0, Skipped: 0, Time elapsed: 135.152 s <<< FAILURE! - in org.cloudfoundry.client.v3.ServiceBindingsTest
[ERROR] getDetails  Time elapsed: 1.003 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext({foo=bar})" failed (expected: onNext({foo=bar}); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

[ERROR] deleteForManagedService  Time elapsed: 12.318 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext(true)" failed (expected: onNext(true); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

[ERROR] createServiceKeyFromManagedServiceInstance  Time elapsed: 11.738 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext(test-service-key-knfej)" failed (expected: onNext(test-service-key-knfej); actual: onComplete())

[ERROR] createAppBindingFromUserProvidedServiceInstance  Time elapsed: 0.442 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext({test-label=test-label-value})" failed (expected: onNext({test-label=test-label-value}); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

[ERROR] get  Time elapsed: 0.452 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext({test-label=test-label-value})" failed (expected: onNext({test-label=test-label-value}); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

[ERROR] deleteForUserProvidedService  Time elapsed: 0.456 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNext(false)" failed (expected: onNext(false); actual: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

[ERROR] listForApplication  Time elapsed: 0.445 s  <<< FAILURE!
java.lang.AssertionError: expectation "expectNextCount(1)" failed (expected: count = 1; actual: counted = 0; signal: onError(org.cloudfoundry.reactor.util.JsonParsingException: Found unexpected property last_operation in payload for org.cloudfoundry.client.v3.servicebindings.ServiceBindingResource$Json))

Still looking at the PR and playing with it, but was curious why override deserialize here. Did you hit a specific issue there, was the default implementation not deserializing the payload properly?

Thanks

@dmikusa
Copy link
Contributor

dmikusa commented Jul 27, 2022

Also, from what I can tell, it looks like CAPI 1.109.0, which is what we have in Tanzu Application Server 2.11 does contain the API for service binding credentials. I would suggest marking the test as @IfCloudFoundryVersion(greaterThanOrEqualTo = CloudFoundryVersion.PCF_2_11), unless you've had issues with 2.11 or 2.12 that are corrected in 2.13. Thanks

@radito3
Copy link
Contributor Author

radito3 commented Jul 27, 2022

Hi @dmikusa-pivotal, thanks for pointing out the errors, I had 2 small issues. They are fixed now.
Could you take another look?
Thanks,
Rangel

@dmikusa
Copy link
Contributor

dmikusa commented Jul 27, 2022

Excellent, thanks. Tests are passing, merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants